Turn on documentation rules in pre-commit #406

emolter · 2025-02-19T16:25:26Z

This PR turns on the Ruff "D" rules and numpydoc-validation pre-commit hook in order to ensure that all docstrings in stdatamodels follow the same style as jwst.

In order to bring the repository into compliance, I had to add a lot of brand-new docstrings to specific reference models, and these were not always models that I understand very well - during review, please pay special attention to these.

A few docstrings, specifically in wcs_ref_models.py, remain that I couldn't easily figure out how to populate because I didn't know enough about how the distortion models work. Hopefully one of the reviewers can provide clarity there.

Tasks

update or add relevant tests
update relevant docstrings and / or docs/ page
Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
- write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
- run jwst regression tests with this branch installed ("git+https://github.com/<fork>/stdatamodels@<branch>")

news fragment change types...

changes/<PR#>.feature.rst: new feature
changes/<PR#>.bugfix.rst: fixes an issue
changes/<PR#>.doc.rst: documentation change
changes/<PR#>.removal.rst: deprecation or removal of public API
changes/<PR#>.misc.rst: infrastructure or miscellaneous change

codecov · 2025-02-19T16:28:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.44%. Comparing base (4e83608) to head (5331100).
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   78.16%   78.44%   +0.27%     
==========================================
  Files         115      109       -6     
  Lines        5144     5084      -60     
==========================================
- Hits         4021     3988      -33     
+ Misses       1123     1096      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

emolter · 2025-02-19T16:53:57Z

src/stdatamodels/jwst/datamodels/wcs_ref_models.py

@nden Would you please take a look at all the TODOs in this file? Or if you're unfamiliar with these, maybe you know who is familiar?

braingram

Thanks for putting this together. I haven't finished reviewing the changes but figured I'd submit the comments so far.

I did not yet review:

jwst.datamodels.wcs_ref_models.py
jwst.transforms.models

braingram · 2025-02-21T22:03:35Z

src/stdatamodels/fits_support.py

-    Returns `True` if the given `key` is a built-in FITS keyword, i.e.
-    a keyword that is managed by ``astropy.io.fits`` and we wouldn't
-    want to propagate through the `_extra_fits` mechanism.
+    Check if key is a fits builtin.


Suggested change

Check if key is a fits builtin.

Check if key is a FITS builtin.

braingram · 2025-02-22T12:37:05Z

pyproject.toml

@@ -119,6 +119,27 @@ select = [
    "E722",
 ]

+[tool.numpydoc_validation]
+checks = [
+    "all",


Since all is here do we need the other rules?

This syntax gives us "all" except the rules added below.

Thanks for the clarification. Would you add a comment here describing that?

braingram · 2025-02-22T18:08:16Z

src/stdatamodels/fits_support.py

+
+    Parameters
+    ----------
+    hdulist : weakref.ReferenceType


Is this a weakref? From the usage here it looks to be an HDUList proper:

stdatamodels/src/stdatamodels/fits_support.py

Line 216 in 0d6c26d

hdu = get_hdu(hdulist, hdu_name, index=index)

you're right it's a HDUlist, will update

braingram · 2025-02-22T18:14:10Z

src/stdatamodels/fits_support.py

@@ -742,7 +813,25 @@ def from_fits(hdulist, schema, context, skip_fits_update=None, **kwargs):

 def from_fits_asdf(hdulist, ignore_unrecognized_tag=False, **kwargs):
    """
-    Wrap asdf call to extract optional arguments
+    Wrap asdf call to extract optional arguments.


I'm not sure this docstring summary makes sense any more. This is much more than a "wrap".

Suggested change

Wrap asdf call to extract optional arguments.

Open the ASDF extension from a FITS hdulist.

If the ASDF extension doesn't exist a new asdf.AsdfFile

instance will be returned.

braingram · 2025-02-22T18:21:13Z

src/stdatamodels/fits_support.py

@@ -911,6 +1011,19 @@ def fits_hash(hdulist):


 def get_short_doc(schema):
+    """
+    Get the short description from the schema.


Suggested change

Get the short description from the schema.

Combine the title and description of a schema.

Does this need a docstring? Can we instead make it private? The code seems messy and writing a docstring that describes the behavior would make this docstring even longer (and it's already longer than the code).

yeah I think it's fine to make private, will do

braingram · 2025-02-24T13:25:01Z

src/stdatamodels/properties.py

@@ -330,6 +354,8 @@ def instance(self):


 class ObjectNode(Node):
+    """A schema-validated dictionary-like structure."""


Suggested change

"""A schema-validated dictionary-like structure."""

"""A dictionary-like Node."""

braingram · 2025-02-24T13:25:17Z

src/stdatamodels/properties.py

@@ -410,6 +436,8 @@ def items(self):


 class ListNode(Node):
+    """A list of schema-validated objects."""


Suggested change

"""A list of schema-validated objects."""

"""A list-like Node."""

braingram · 2025-02-24T13:25:48Z

src/stdatamodels/properties.py

@@ -144,9 +154,21 @@ def _as_fitsrec(val):

 def _get_schema_type(schema):
    """
+    Find the type of a schema and its subschemas.


Suggested change

Find the type of a schema and its subschemas.

Find the type or types used by a schema.

braingram · 2025-02-24T13:26:56Z

src/stdatamodels/properties.py

+    schema_type : str
+        The type of the schema, or 'mixed' if the schema has multiple types.


Suggested change

schema_type : str

The type of the schema, or 'mixed' if the schema has multiple types.

str

The type used by a schema or 'mixed' if the schema has multiple types.

braingram · 2025-02-24T13:32:17Z

src/stdatamodels/model_base.py

-        """
-        Re-validate the model instance against its schema
-        """
+        """Re-validate the model instance against its schema."""


Suggested change

"""Re-validate the model instance against its schema."""

"""Validate the model instance against its schema."""

src/stdatamodels/jwst/datamodels/abvega_offset.py

emolter commented Feb 19, 2025

View reviewed changes

emolter marked this pull request as ready for review February 19, 2025 16:56

emolter requested a review from a team as a code owner February 19, 2025 16:56

emolter requested review from melanieclarke, braingram, tapastro and nden February 19, 2025 16:59

braingram reviewed Feb 24, 2025

View reviewed changes

emolter force-pushed the numpydoc-validation branch from d89463b to 21e9014 Compare February 24, 2025 18:21

emolter added 6 commits February 27, 2025 09:54

Update configuration for ruff and numpydoc to match jwst

479ade6

Add ruff docstring rules

66f6c40

Add numpydoc formatting to base stdatamodels package

3dcf242

Turn on numpydoc for jwst/ folder

1738974

changelog and small fixes from self-review

638f3e8

Fixes from Brett's initial review

5331100

emolter force-pushed the numpydoc-validation branch from 21e9014 to 5331100 Compare February 27, 2025 15:01

emolter commented Feb 27, 2025

View reviewed changes

src/stdatamodels/jwst/datamodels/abvega_offset.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on documentation rules in pre-commit #406

Turn on documentation rules in pre-commit #406

emolter commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading

emolter Feb 19, 2025

braingram left a comment

braingram Feb 21, 2025

braingram Feb 22, 2025

emolter Feb 24, 2025

braingram Feb 24, 2025

braingram Feb 22, 2025

emolter Feb 24, 2025

braingram Feb 22, 2025

braingram Feb 22, 2025

emolter Feb 24, 2025

braingram Feb 24, 2025

braingram Feb 24, 2025

braingram Feb 24, 2025

braingram Feb 24, 2025

braingram Feb 24, 2025

	Check if key is a fits builtin.
	Check if key is a FITS builtin.

-    Wrap asdf call to extract optional arguments.
+    Open the ASDF extension from a FITS hdulist.
+    If the ASDF extension doesn't exist a new asdf.AsdfFile
+    instance will be returned.

	Get the short description from the schema.
	Combine the title and description of a schema.

		@@ -330,6 +354,8 @@ def instance(self):


		class ObjectNode(Node):
		"""A schema-validated dictionary-like structure."""

	"""A schema-validated dictionary-like structure."""
	"""A dictionary-like Node."""

		@@ -410,6 +436,8 @@ def items(self):


		class ListNode(Node):
		"""A list of schema-validated objects."""

	"""A list of schema-validated objects."""
	"""A list-like Node."""

	Find the type of a schema and its subschemas.
	Find the type or types used by a schema.

		schema_type : str
		The type of the schema, or 'mixed' if the schema has multiple types.

	"""Re-validate the model instance against its schema."""
	"""Validate the model instance against its schema."""

Turn on documentation rules in pre-commit #406

Are you sure you want to change the base?

Turn on documentation rules in pre-commit #406

Conversation

emolter commented Feb 19, 2025 • edited Loading

Tasks

codecov bot commented Feb 19, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

braingram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emolter commented Feb 19, 2025 •

edited

Loading

codecov bot commented Feb 19, 2025 •

edited

Loading