8000 JP-2261: Allow user-specified custom WCS parameters in the resample step by mcara · Pull Request #6364 · spacetelescope/jwst · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

JP-2261: Allow user-specified custom WCS parameters in the resample step #6364

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 9 commits into from
Oct 25, 2021
Merged

JP-2261: Allow user-specified custom WCS parameters in the resample step #6364

merged 9 commits into from
Oct 25, 2021

Conversation

mcara
Copy link
Member
@mcara mcara commented Sep 23, 2021 •
edited by jdavies-st
Loading

Fixes #6301
Fixes #6358
Resolves JP-2261

@mcara mcara requested a review from jdavies-st September 23, 2021 19:35
@mcara mcara self-assigned this Sep 23, 2021
@mcara
Copy link
Member Author
mcara commented Sep 23, 2021

@jdavies-st I think output_wcs.data_size could be replaced with array_shape which is now populated correctly. The question is: should I do this in this PR or to have a separate PR just for that.

@jdavies-st
Copy link
Collaborator

@jdavies-st I think output_wcs.data_size could be replaced with array_shape which is now populated correctly. The question is: should I do this in this PR or to have a separate PR just for that.

Feel free to do it here. It's a good improvement. Attaching an unofficial attribute to a WCS object was always dicey.

jdavies-st
jdavies-st reviewed Sep 23, 2021
View reviewed changes
@jdavies-st jdavies-st changed the title Allow user-specified custom WCS parameters in the resample step JP-2261: Allow user-specified custom WCS parameters in the resample step Sep 23, 2021
@jdavies-st jdavies-st added this to the Build 7.9 milestone Sep 23, 2021
@codecov
Copy link
codecov bot commented Sep 25, 2021 •
edited
Loading

Codecov Report

Merging #6364 (410fbd1) into master (1c26772) will increase coverage by 0.01%.
The diff coverage is 88.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6364      +/-   ##
==========================================
+ Coverage   78.58%   78.59%   +0.01%     
==========================================
  Files         408      408              
  Lines       35058    35567     +509     
==========================================
+ Hits        27550    27954     +404     
- Misses       7508     7613     +105
Flag Coverage Δ *Carryforward flag
nightly 78.54% <90.54%> (ø) Carriedforward from a1242f4
unit 56.37% <86.76%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
jwst/resample/resample_utils.py 90.22% <82.60%> (+3.55%) ⬆️
jwst/resample/resample_step.py 87.63% <83.33%> (-1.56%) ⬇️
jwst/resample/resample_spec.py 88.88% <87.50%> (-8.20%) ⬇️
jwst/assign_wcs/util.py 78.90% <90.90%> (-2.61%) ⬇️
jwst/resample/resample.py 94.85% <100.00%> (-1.87%) ⬇️

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 1c26772...410fbd1. Read the comment docs.

hbushouse
hbushouse reviewed Sep 28, 2021
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.

Looks good to me. Before we finalize this, however, it would be useful to post info in the JP-2261 ticket as to what new parameters are being added, so that the INS folks have a chance to review and comment.

@stscijgbot-jp stscijgbot-jp mentioned this pull request Oct 7, 2021
Closed
@mcara mcara requested review from jdavies-st and hbushouse October 8, 2021 01:30
jdavies-st
jdavies-st reviewed Oct 8, 2021
View reviewed changes
hbushouse
hbushouse reviewed Oct 12, 2021
View reviewed changes
@mcara
Copy link
Member Author
mcara commented Oct 16, 2021 •
edited
Loading

PR spacetelescope/stpipe#33 fixes the issue with list arguments.

I have also discussed offline with @jdavies-st the issue of array_shape vs a property of the Resample* class itself and the decision was made to keep proposed in this PR change.

@hbushouse
Copy link
Collaborator

So it looks like all comments have been resolved, such that once spacetelescope/stpipe#33 is finalized/merged, this PR can be declared done, right?

@nden nden requested review from hbushouse and jdavies-st October 21, 2021 13:38
jdavies-st
jdavies-st approved these changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator
@jdavies-st jdavies-st left a comment

Choose a reason for hiding this comment

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

Looks great. And nice that we're using the APE 14 features that attaches array_shape attribute to the WCS object. I didn't know such an attribute existed before this PR.

🚀

hbushouse
hbushouse approved these changes Oct 21, 2021
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.

Code looks good. But given that we're changing/adding step arguments, the step docs should also be updated to include those new params (https://jwst-pipeline.readthedocs.io/en/latest/jwst/resample/arguments.html). Can be done in a separate PR, if you want, or add it here.

@nden
Copy link
Collaborator
nden commented Oct 24, 2021 •
edited
Loading

@mcara needs a rebase to resolve the conflict.

@mcara
Copy link
Member Author
mcara commented Oct 25, 2021

@hbushouse I will add documentation here.

@mcara mcara force-pushed the support-outwcs-user-pars branch from d6acecb to a1242f4 Compare October 25, 2021 16:16
@mcara
Copy link
Member Author
mcara commented Oct 25, 2021

@hbushouse All tests used to pass before I rebased. Now --pyargs seems to be failing. I do not believe this is related to this PR.

@mcara
Copy link
Member Author
mcara commented Oct 25, 2021 6D40

@hbushouse I just documented arguments. Please review and feel free to merge.

@hbushouse hbushouse merged commit b69aa67 into spacetelescope:master Oct 25, 2021
nden pushed a commit to nden/jwst that referenced this pull request Nov 2, 2021
@mcara @nden
JP-2261: Allow user-specified custom WCS parameters in the resample s…
e2a69a1 
…tep (spacetelescope#6364)

* Allow user-specified custom WCS parameters in the resample step

* remove iraf starfinder

* use lists for some parameters

* remove itertools import - pep8

* remove data_size and fix failing tests

* Add support for user specified absolute pixel scale for output of resample

* Fix bug in the test

* Fix pep8

* Update arguments docs for the resample step
nden pushed a commit to nden/jwst that referenced this pull request Nov 2, 2021
@mcara @nden
JP-2261: Allow user-specified custom WCS parameters in the resample s…
27b7890 
…tep (spacetelescope#6364)

* Allow user-specified custom WCS parameters in the resample step

* remove iraf starfinder

* use lists for some parameters

* remove itertools import - pep8

* remove data_size and fix failing tests

* Add support for user specified absolute pixel scale for output of resample

* Fix bug in the test

* Fix pep8

* Update arguments docs for the resample step
loicalbert pushed a commit to talensgj/jwst that referenced this pull request Nov 5, 2021
@mcara @loicalbert
JP-2261: Allow user-specified custom WCS parameters in the resample s…
126b946 
…tep (spacetelescope#6364)

* Allow user-specified custom WCS parameters in the resample step

* remove iraf starfinder

* use lists for some parameters

* remove itertools import - pep8

* remove data_size and fix failing tests

* Add support for user specified absolute pixel scale for output of resample

* Fix bug in the test

* Fix pep8

* Update arguments docs for the resample step
@nden nden mentioned this pull request Mar 30, 2022
Closed
@mcara mcara deleted the support-outwcs-user-pars branch April 8, 2023 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbushouse hbushouse hbushouse approved these changes

@jdavies-st jdavies-st jdavies-st approved these changes

Assignees

@mcara mcara

Labels
assign_wcs enhancement resample
Projects
None yet
Milestone
Build 7.9
Development

Successfully merging this pull request may close these issues.

Contradictory meaning of pixel_scale_ratio in resample step Allow necessary image drizzling parameters to be passed into resample step
4 participants
@mcara @jdavies-st @hbushouse @nden
0