Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move find_fits_keyword to fits_support #396

Closed
wants to merge 2 commits into from

Conversation

emolter
Copy link
Contributor

@emolter emolter commented Feb 11, 2025

Closes #33

This PR makes a small reorganization, moving the FITS-format-specific function find_fits_keyword out of schema/ and into fits_support/. Note that find_fits_keyword is also a method of a DataModel, and that method is unchanged except that it internally calls from elsewhere in the codebase.

That is, the documented way to call this function, e.g., model.find_fits_keyword('DATE-OBS'), is unchanged.

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

Copy link

codecov bot commented Feb 11, 2025

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.16%. Comparing base (4e83608) to head (b951636).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/stdatamodels/fits_support.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #396   +/-   ##
=======================================
  Coverage   78.16%   78.16%           
=======================================
  Files         115      115           
  Lines        5144     5144           
=======================================
  Hits         4021     4021           
  Misses       1123     1123           

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

@emolter
Copy link
Contributor Author

emolter commented Feb 11, 2025

This function does not appear to be used at all in JWST, and indeed it's documented that it is not supposed to be used there, but running regression tests anyway just in case https://github.com/spacetelescope/RegressionTests/actions/runs/13254071328

@emolter
Copy link
Contributor Author

emolter commented Feb 11, 2025

@braingram @tapastro Would it be better to deprecate schemas.find_fits_keyword()? On the one hand, it's theoretically possible someone could be calling from stdatamodels.schemas import find_fits_keyword. On the other hand, it seems unlikely to me anyone actually is doing so, since the documented way to call this functionality is as a method on the datamodels, not to call this function. In jwst we implicitly assume anything not in the __all__ is not "public" and requires no deprecation cycle, is that typically the same in stdatamodels?

@braingram
Copy link
Collaborator

I wouldn't consider stdatamodels.schema as part of the public API since it's not in the documentation:
https://stdatamodels.readthedocs.io/en/latest/api.html
Since it's not documented I don't think any deprecation is needed.

I also don't have any objection to keeping it in schema. It's a function that walks a schema looking for a particular schema keyword (fits_keyword). It is related to FITS since the fits_keyword refers to a FITS keyword but it's not needed for writing datamodels to FITS files (which I believe is the goal of fits_support).

@emolter
Copy link
Contributor Author

emolter commented Feb 11, 2025

I am also happy just closing the issue, sound like that would be your preference?

@emolter emolter closed this Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move FITS-related functions from schema.py to fits_support.py
2 participants