Miscellaneous typos and code cleaning #264

rvasav26 · 2025-06-30T16:30:20Z

This addresses some typos in the docs as well as a few redundant code lines. Additionally, we rename a few methods in _BasePCov and _BaseKPCov. Finally, we change test_pcovc to use a multiclass dataset for testing.

Contributor (creator of PR) checklist

Tests updated (for new features and bugfixes)?
Documentation updated (for new features)?
Issue referenced (for PRs that solve an issue)?

For Reviewer

CHANGELOG updated if important change?

📚 Documentation preview 📚: https://scikit-matter--264.org.readthedocs.build/en/264/

cajchristian

These look good. Maybe we should have Philip or Rosy review because they are more knowledgeable with formatting and coding standards than I am.

PicoCentauri

Thanks for the cleanup. I have two comments

I would rephrase the "Default = XXX" to propper sentences
See my comment on how to typeset inline code in the docs. You can also always render the docs locally with tox -e docs or check the read the docs link in the PR conversation.

src/skmatter/decomposition/_pcov.py

PicoCentauri · 2025-07-15T08:59:36Z

src/skmatter/decomposition/_pcovc.py

-        whether to compute the PCovC in `sample` or `feature` space
-        default=`sample` when :math:`{n_{samples} < n_{features}}` and
+        whether to compute the PCovC in `sample` or `feature` space.
+        Default = `sample` when :math:`{n_{samples} < n_{features}}` and


This looks a bit weird for me. Maybe write a "real" sentence like

Suggested change

Default = `sample` when :math:`{n_{samples} < n_{features}}` and

The default value is `sample` when :math:`{n_{samples} < n_{features}}` and

or similar.

Did you push the changes. Somehow I don't see them here?

PicoCentauri · 2025-07-15T09:01:22Z

src/skmatter/decomposition/_pcovc.py

        `feature` when :math:`{n_{features} < n_{samples}}`

-     classifier: `estimator object` or `precomputed`, default=None
+    classifier: `estimator object` or `precomputed`, default=None


If you want to do a code highlighting use double ticks ``. Our documentation pages use rst where inline codes are highlighted with double ticks. Sorry for the confusion and this very subtle difference between markdown and rst.

Ok, that makes sense. I think the reason I have single ticks is to match how PCovR/KPCovR docstrings handle `precomputed`, but I will keep that in mind

PicoCentauri · 2025-07-15T09:01:33Z

src/skmatter/decomposition/_pcovc.py

-        whether to compute the PCovC in `sample` or `feature` space
-        default=`sample` when :math:`{n_{samples} < n_{features}}` and
+        whether to compute the PCovC in `sample` or `feature` space.
+        Default = `sample` when :math:`{n_{samples} < n_{features}}` and


same as above

PicoCentauri

Cool! Thanks for the updates. I have some final minor suggestions to improve the flow when reading the docs.

PicoCentauri · 2025-07-17T06:32:12Z

src/skmatter/decomposition/_pcovr.py

@@ -227,11 +227,12 @@ def fit(self, X, Y, W=None):
            regressed form of the properties, :math:`{\mathbf{\hat{Y}}}`.

        W : numpy.ndarray, shape (n_features, n_properties)
-            Regression weights, optional when regressor= `precomputed`. If not
+            Regression weights, optional when regressor = `precomputed`. If not


Suggested change

Regression weights, optional when regressor = `precomputed`. If not

Regression weights, optional when regressor is ``precomputed``. If not

PicoCentauri · 2025-07-17T06:32:41Z

src/skmatter/decomposition/_pcovr.py

@@ -126,7 +126,7 @@ class PCovR(RegressorMixin, MultiOutputMixin, _BasePCov):
        Must be of range [0.0, infinity).

    space: {'feature', 'sample', 'auto'}, default='auto'
-        whether to compute the PCovR in `sample` or `feature` space default=`sample`
+        whether to compute the PCovR in `sample` or `feature` space. Default = `sample`


Suggested change

whether to compute the PCovR in `sample` or `feature` space. Default = `sample`

whether to compute the PCovR in `sample` or `feature` space. Default is equal to ``sample``

PicoCentauri · 2025-07-17T06:33:16Z

src/skmatter/decomposition/_pcovr.py

+        The default is = `sample` when :math:`{n_{samples} < n_{features}}`
+        and `feature` when :math:`{n_{features} < n_{samples}}`


Suggested change

The default is = `sample` when :math:`{n_{samples} < n_{features}}`

and `feature` when :math:`{n_{features} < n_{samples}}`

The default is equal to ``sample`` when :math:`{n_{samples} < n_{features}}`

and ``feature`` when :math:`{n_{features} < n_{samples}}`

PicoCentauri · 2025-07-17T06:33:55Z

src/skmatter/decomposition/_pcovc.py

+        whether to compute the PCovC in `sample` or `feature` space.
+        The default is = `sample` when :math:`{n_{samples} < n_{features}}`
+        and `feature` when :math:`{n_{features} < n_{samples}}`


Suggested change

whether to compute the PCovC in `sample` or `feature` space.

The default is = `sample` when :math:`{n_{samples} < n_{features}}`

and `feature` when :math:`{n_{features} < n_{samples}}`

whether to compute the PCovC in ``sample`` or ``feature`` space.

The default is equal to ``sample`` when :math:`{n_{samples} < n_{features}}`

and ``feature`` when :math:`{n_{features} < n_{samples}}`

PicoCentauri · 2025-07-17T06:34:16Z

src/skmatter/decomposition/_pcovc.py

        `feature` when :math:`{n_{features} < n_{samples}}`

-     classifier: `estimator object` or `precomputed`, default=None
+    classifier: `estimator object` or `precomputed`, default=None


Suggested change

classifier: `estimator object` or `precomputed`, default=None

classifier: ``estimator object`` or ``precomputed``, default=None

PicoCentauri · 2025-07-17T06:34:45Z

src/skmatter/decomposition/_pcovc.py

-        whether to compute the PCovC in `sample` or `feature` space
-        default=`sample` when :math:`{n_{samples} < n_{features}}` and
+        whether to compute the PCovC in `sample` or `feature` space.
+        Default = `sample` when :math:`{n_{samples} < n_{features}}` and


Did you push the changes. Somehow I don't see them here?

rvasav26 · 2025-07-17T18:07:10Z

Cool! Thanks for the updates. I have some final minor suggestions to improve the flow when reading the docs.

Great! Everything should be added now. I think a few changes were left out from the previous commit.

PicoCentauri · 2025-07-17T20:24:53Z

I am happy. We can merge if @cajchristian is as well.

cajchristian

I am happy too!

rvasav26 force-pushed the pcovc-edits branch from f47da03 to f4b2c11 Compare June 30, 2025 17:27

rvasav26 marked this pull request as ready for review June 30, 2025 17:35

rvasav26 requested a review from cajchristian June 30, 2025 17:36

cajchristian reviewed Jul 14, 2025

View reviewed changes

PicoCentauri reviewed Jul 15, 2025

View reviewed changes

rvasav26 added 6 commits July 15, 2025 12:15

Minor typos

82ff826

Changes to PCovC tests

46d2fca

Changes to KPCovC tests

25a96f6

Minor reorganizing and typos

64438a9

Trying some things for codecov

9f0a99e

Docstring edits from suggestions

91f80b1

rvasav26 force-pushed the pcovc-edits branch from df6a2f0 to 91f80b1 Compare July 15, 2025 17:41

rvasav26 requested a review from PicoCentauri July 16, 2025 21:55

PicoCentauri reviewed Jul 17, 2025

View reviewed changes

More docstring touch-ups

1f91002

PicoCentauri approved these changes Jul 17, 2025

View reviewed changes

cajchristian approved these changes Jul 17, 2025

View reviewed changes

rvasav26 merged commit eb9919e into main Jul 17, 2025
10 checks passed

rvasav26 deleted the pcovc-edits branch July 17, 2025 21:30

	Default = `sample` when :math:`{n_{samples} < n_{features}}` and
	The default value is `sample` when :math:`{n_{samples} < n_{features}}` and

	Regression weights, optional when regressor = `precomputed`. If not
	Regression weights, optional when regressor is ``precomputed``. If not

	whether to compute the PCovR in `sample` or `feature` space. Default = `sample`
	whether to compute the PCovR in `sample` or `feature` space. Default is equal to ``sample``

		The default is = `sample` when :math:`{n_{samples} < n_{features}}`
		and `feature` when :math:`{n_{features} < n_{samples}}`

	classifier: `estimator object` or `precomputed`, default=None
	classifier: ``estimator object`` or ``precomputed``, default=None

Miscellaneous typos and code cleaning #264

Miscellaneous typos and code cleaning #264

Uh oh!

Conversation

rvasav26 commented Jun 30, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor (creator of PR) checklist

For Reviewer

Uh oh!

cajchristian left a comment

Choose a reason for hiding this comment

Uh oh!

PicoCentauri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PicoCentauri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvasav26 commented Jul 17, 2025

Uh oh!

PicoCentauri commented Jul 17, 2025

Uh oh!

cajchristian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rvasav26 commented Jun 30, 2025 •

edited by github-actions bot

Loading