8000 Open specwcs reffile with context manager by jdavies-st · Pull Request #6160 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Open specwcs reffile with context manager #6160

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

Merged

Conversation

jdavies-st
Copy link
Collaborator
@jdavies-st jdavies-st commented Jun 21, 2021

This PR fixes a problem we saw in a unit test where the specwcs reffile was incompletely downloaded from CRDS and so asdf.open() failed. When it failed, it was not closed, as it never went through __exit__.

Not sure if this is a problem with the with context manager in asdf.open(), i.e. is heavy-lifting happening in AsdfFile.__init__ that should be happening in AsdfFile.__enter__? Not sure.

Regardless, in this case, as soon as asdf.open(file) was called, it opens it without using __enter__. Is this a bug or feature in asdf @eslavich ? Or just a small bug here.

Separate issue filed in asdf-format/asdf#1006

Also cleaned up a try/except block below. velosys is guaranteed to be an attribute as it is in the schema.

Checklist

  • Tests
  • Documentation
  • Change log
  • Milestone
  • Label(s)

@jdavies-st jdavies-st added this to the Build 7.9 milestone Jun 21, 2021
@codecov
Copy link
codecov bot commented Jun 21, 2021

Codecov Report

Merging #6160 (dbd9575) into master (9fde762) will increase coverage by 0.00%.
The diff coverage is 53.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6160   +/-   ##
=======================================
  Coverage   77.15%   77.15%           
=======================================
  Files         402      402           
  Lines       34369    34436   +67     
=======================================
+ Hits        26517    26569   +52     
- Misses       7852     7867   +15     
Flag Coverage Δ *Carryforward flag
nightly 77.16% <57.14%> (ø) Carriedforward from 19276d4
unit 55.20% <0.00%> (-0.01%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
jwst/assign_wcs/niriss.py 75.96% <53.33%> (-1.89%) ⬇️
jwst/lib/suffix.py 97.01% <0.00%> (+1.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fde762...dbd9575. Read the comment docs.

@jdavies-st jdavies-st requested review from nden and eslavich June 21, 2021 18:46
Comment on lines -150 to -153
try:
velosys = input_model.meta.wcsinfo.velosys
except AttributeError:
pass
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for a try/except block here as it will never fail to have this attribute since it is in the wcsinfo schema.

@jdavies-st jdavies-st requested review from eslavich and removed request for nden July 1, 2021 19:23
Copy link
Contributor
@eslavich eslavich left a comment

Choose a reason for hiding this comment

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

LGTM

@jdavies-st jdavies-st merged commit 9f4eda3 into spacetelescope:master Jul 1, 2021
@jdavies-st jdavies-st deleted the fix-context-manager-with-s3 branch July 1, 2021 20:01
loicalbert pushed a commit to talensgj/jwst that referenced this pull request Nov 5, 2021
* Open specwcs reffile with context manager

* Update CHANGES.rst

* Address comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0