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

update_slit_metadata tests and non-existing slitled_id case catching #73

Closed
wants to merge 2 commits into from

Conversation

KostasValeckas
Copy link
Contributor

This is a premature PR on the utils tests, but as I found one branch in code that needs attention I want to send it right away.

Tests for all 'if' branches of the update_slit_metadata added.

The last if branch would never be reached as it is now, since it is only triggered if the slit.slitlet_id attribute is not existing. However, the jwst datamodel automatically sets it to 0 when it is non-existing or None, as can be seen recreated here:

>>> import jwst.datamodels
>>> slit = jwst.datamodels.open("/home/kostasvaleckas/Documents/DAWN/msaexp/msaexp/tests/data/jw01345062001_03101_00001_nrs2_phot.138.1345_933.fits")
>>> slit.slitlet_id
138
>>> slit.slitlet_id = None
>>> slit.slitlet_id
0
>>> slit.slitlet_id = 138
>>> slit.slitlet_id
138
>>> del slit.slitlet_id
>>> slit.slitlet_id
0

I therefore suggest a fix for checking it for value 0 as a proxy for it not being set, but I am not sure whether this is a good fix.

All tests pass.

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.16%. Comparing base (d3874fe) to head (31a8b54).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #73      +/-   ##
==========================================
+ Coverage   65.89%   66.16%   +0.26%     
==========================================
  Files          15       15              
  Lines        6685     6720      +35     
==========================================
+ Hits         4405     4446      +41     
+ Misses       2280     2274       -6     

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

@gbrammer
Copy link
Owner

I understand why that cause will never be triggered as currently implemented. However, it's possible that the jwst.datamodels behavior could change in the future with otherwise invisible effects that could cause a fault in some rare edge case.

We don't have to be too overzealous about making the test coverage 100%, especially if doing so requires keeping a bunch more test data around in the repository or if the test actions start taking a long time to run.

@KostasValeckas
Copy link
Contributor Author

KostasValeckas commented May 22, 2024

Roger that - thanks for the feedback.

I will close this PR as a new one with a different approach would make more sense.

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.

3 participants