Skip to content

Conversation

@emolter
Copy link
Contributor

@emolter emolter commented Jun 25, 2025

Closes #506

This PR hard-codes memmap to False when reading FITS and ASDF, and emits a warning when a user attempts to use memory mapping.

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

@emolter
Copy link
Contributor Author

emolter commented Jun 25, 2025

@codecov
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.07%. Comparing base (d65ce14) to head (c817ddb).
⚠️ Report is 175 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   81.02%   81.07%   +0.04%     
==========================================
  Files         110      110              
  Lines        5422     5431       +9     
==========================================
+ Hits         4393     4403      +10     
+ Misses       1029     1028       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@emolter
Copy link
Contributor Author

emolter commented Jun 25, 2025

@drlaw1558 How do these deprecation messages look to you?

@emolter emolter marked this pull request as ready for review June 25, 2025 16:33
@emolter emolter requested a review from a team as a code owner June 25, 2025 16:33
@drlaw1558
Copy link
Contributor

LG2M

@emolter emolter marked this pull request as draft June 25, 2025 18:49
@emolter
Copy link
Contributor Author

emolter commented Jun 25, 2025

This PR needs to wait until spacetelescope/jwst#9593 is merged; otherwise the warnings are emitted whenever a ModelContainer is initialized

@emolter
Copy link
Contributor Author

emolter commented Jul 7, 2025

@melanieclarke
Copy link
Contributor

Looks good, pending clean regtests.

Did this one have an associated JP ticket? I know we asked INS if it was okay to remove the argument, did we hear a final word on that?

@emolter
Copy link
Contributor Author

emolter commented Jul 7, 2025

It did not have an associated ticket. David did bring it up in a JP coordination meeting and then subsequently gave me the go-ahead after hearing that nobody knew memmap was even an option - at least, that's how I interpreted it.

@emolter
Copy link
Contributor Author

emolter commented Jul 7, 2025

regtests are clean

@emolter emolter marked this pull request as ready for review July 7, 2025 18:43
Copy link
Contributor

@melanieclarke melanieclarke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I'll hold off on approval to see if @braingram wants to review/comment.

@braingram
Copy link
Collaborator

Let's make the next version of stdatamodels 4.0.0 since this is a breaking change.

@emolter emolter merged commit c36cf1c into spacetelescope:main Jul 8, 2025
19 checks passed
@emolter emolter deleted the remove-memmap branch July 8, 2025 13:10
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.

Opening models that contain unsigned data does not work with memmap=True

4 participants