8000 fix definition of pixel ratio by cshanahan1 · Pull Request #7048 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix definition of pixel ratio #7048

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Sep 20, 2022
Merged

fix definition of pixel ratio #7048

merged 5 commits into from
Sep 20, 2022

Conversation

cshanahan1
Copy link
Collaborator
@cshanahan1 cshanahan1 commented Sep 16, 2022 •
edited by hbushouse
Loading

Fixes an error introduced by #7033 where the pixel scale ratio was incorrectly defined - it is now correctly computed as a ratio of pixel areas.

Checklist for maintainers

  • added entry in CHANGES.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant milestone
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below.
    How to run regression tests on a PR
  • Make sure the JIRA ticket is resolved properly

@cshanahan1 cshanahan1 requested a review from mcara September 16, 2022 20:29
mcara
mcara approved these changes Sep 16, 2022
View reviewed changes
Copy link
Member
@mcara mcara left a comment

Choose a reason for hiding this comment

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

LGTM.

mcara
mcara reviewed Sep 16, 2022
View reviewed changes
@codecov
Copy link
codecov bot commented Sep 16, 2022 •
edited
Loading

Codecov Report

Base: 79.60% // Head: 79.61% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (35aee62) compared to base (3062ef3).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #7048   +/-   ##
=======================================
  Coverage   79.60%   79.61%           
=======================================
  Files         411      411           
  Lines       37298    37298           
=======================================
+ Hits        29691    29694    +3     
+ Misses       7607     7604    -3
Flag Coverage Δ
nightly 79.59% <100.00%> (+<0.01%) ⬆️
unit 53.27% <100.00%> (ø)
Impacted Files Coverage Δ
jwst/resample/resample_step.py 87.94% <100.00%> (+1.41%) ⬆️
jwst/regtest/regtestdata.py 84.86% <0.00%> (-0.46%) ⬇️
jwst/model_blender/blendmeta.py 83.76% <0.00%> (+1.70%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mcara
Copy link
Member
mcara commented Sep 17, 2022

I do not believe failing tests are related to this PR.

@mcara mcara added this to the Build 9.0 milestone Sep 17, 2022
@mcara mcara enabled auto-merge (squash) September 17, 2022 04:36
@mcara mcara disabled auto-merge September 17, 2022 04:37
hbushouse
hbushouse suggested changes Sep 19, 2022
View reviewed changes
Copy link
Collaborator
@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

Found another error in the original PR that was merged. The changes implemented in terms of handling the drizpars reference file are now producing an error when the "get_reference_file" query to CRDS comes back with "N/A" (i.e. there's no drizpars ref file in CRDS applicable to the particular instrument+mode). The check for ref_filename == 'N/A' in line 87 of resample_step.py needs to be executed before the call to get_drizpars is made in line 83. If get_drizpars is called with a ref file name of "N/A", it throws an error.

hbushouse
hbushouse reviewed Sep 20, 2022
View reviewed changes
@hbushouse
Copy link
Collaborator

I do not believe failing tests are related to this PR.

You can believe anything you want, but this was in fact leading to 2 different kinds of errors. One has been fixed, now I'm in the process of fixing the other. Always, always, always run a full regression test. Never rely on the built-in CI tests only.

@cshanahan1
Copy link
Collaborator Author

@hbushouse should i run the regression tests on this branch then before we merge?

@hbushouse
Copy link
Collaborator

@hbushouse should i run the regression tests on this branch then before we merge?

I did one run earlier this morning, which revealed the remaining error (having to do with initializing self.wht_type) and am about to kick off another run now that that problem has been fixed too in the code.

@hbushouse
Copy link
Collaborator

Yay! CI tests have passed and the full regtest run is clean: https://plwishmaster.stsci.edu:8081/job/RT/job/JWST-Developers-Pull-Requests/428/

I'm merging.

hbushouse
hbushouse approved these changes Sep 20, 2022
View reviewed changes
Copy link
Collaborator
@hbushouse hbushouse left a comment

Choose a reason for hiding this comment

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

It's all good now.

@hbushouse hbushouse merged commit 8b1a3c1 into spacetelescope:master Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcara mcara mcara approved these changes

@hbushouse hbushouse hbushouse approved these changes

Assignees

@cshanahan1 cshanahan1

Labels
resample testing
Projects
None yet
Milestone
Build 9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@cshanahan1 @mcara @hbushouse
0