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.

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

Updating wcs fitting to match astropy #29

Merged
merged 2 commits into from Apr 23, 2020
Merged

Updating wcs fitting to match astropy #29

merged 2 commits into from Apr 23, 2020

Conversation

ceb8
Copy link
Collaborator

@ceb8 ceb8 commented Apr 23, 2020

The PR resolves issue #22.

  • Updated the local fit_wcs_from_points to match what is in Astropy.
  • Added additional residual checking.
  • Opened bugfix PR to add the same residual checking in Astropy (BUG: Adding wrap around checking in WCS astropy/astropy#10155).
  • Added check so that for Astropy version >=4.0.1 the Astropy function will be used preferentially (this will be the version my bugfix goes into)

@ceb8 ceb8 added this to the 0.5 milestone Apr 23, 2020
@ceb8 ceb8 modified the milestones: 0.5, 0.6 Apr 23, 2020
@ceb8 ceb8 requested a review from scfleming April 23, 2020 17:07
scfleming
scfleming reviewed Apr 23, 2020
View reviewed changes
astrocut/utils/wcs_fitting.py
w_obj: `~astropy.wcs.WCS`
WCS object
"""
cd = params[0:4]
Copy link
Collaborator

Choose a reason for hiding this comment

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

A little dangerous to access parameters directly on indexes, if params[] changes structure in the future this would need to be updated and could be forgoten about. Minor comment though.

scfleming
scfleming reviewed Apr 23, 2020
View reviewed changes
astrocut/utils/wcs_fitting.py
""" Objective function for fitting SIP.
Parameters
-----------
params : array
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment on passing index of parameter values with an assumed ordering...

scfleming
scfleming approved these changes Apr 23, 2020
View reviewed changes
Copy link
Collaborator

@scfleming scfleming left a comment

Choose a reason for hiding this comment

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

For future: may want to re-consider passing parameters via lists with assumed ordering and sizes, but if functional for now I think that is a minor improvement looking to future-proof the code. Not necessary for this pull request probably...

@ceb8
Copy link
Collaborator Author

ceb8 commented Apr 23, 2020

Thanks @scfleming! I'm not going to change for this PR, but those are great comments, and I'll make a note to look into submitting an improvement to astropy to future proof in that way.

@ceb8 ceb8 merged commit ed4c2d2 into master Apr 23, 2020
@ceb8 ceb8 deleted the astropy_upgrade branch April 23, 2020 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scfleming scfleming scfleming approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ceb8 @scfleming