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

Fix a bug in setting type when converting colsel to ndarray #11431

Merged
merged 3 commits into from
Apr 27, 2021
Merged

Fix a bug in setting type when converting colsel to ndarray #11431

merged 3 commits into from
Apr 27, 2021

Conversation

mcara
Copy link
Contributor

@mcara mcara commented Mar 26, 2021

Fixes #11412

It turns out that colsel was not converted correctly to numpy arrays due to a wrong placement of the type argument.

@mcara mcara added the Bug label Mar 26, 2021
@mcara mcara requested a review from nden March 26, 2021 00:07
@mcara mcara self-assigned this Mar 26, 2021
@github-actions github-actions bot added the wcs label Mar 26, 2021
@mcara
Copy link
Contributor Author

mcara commented Mar 26, 2021

CC: @hamogu

@pllim pllim added this to the v4.0.6 milestone Mar 26, 2021
@pllim pllim requested a review from hamogu March 26, 2021 00:28
@mcara
Copy link
Contributor Author

mcara commented Mar 26, 2021

In the second commit I fixed the same bug in two other functions cylfix and Wcsprm.fix.

@nden
Copy link
Contributor

nden commented Mar 26, 2021

@mcara Looks good. Can you add a test?

@mcara mcara force-pushed the fix-colsel-type-conversion branch from ea8c35a to 8fddb90 Compare March 26, 2021 15:16
hamogu
hamogu reviewed Mar 26, 2021
View reviewed changes
Copy link
Member

@hamogu hamogu left a comment

Choose a reason for hiding this comment

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

I see you added a test. Great! I have two suggestions for you to consider.

astropy/wcs/tests/test_wcs.py Outdated Show resolved Hide resolved
astropy/wcs/tests/test_wcs.py Show resolved Hide resolved
@mcara mcara force-pushed the fix-colsel-type-conversion branch from cd22297 to 56b8de9 Compare March 26, 2021 16:09
@pep8speaks
Copy link

pep8speaks commented Mar 26, 2021 •
edited
Loading

Hello @mcara 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-04-26 22:57:47 UTC

@mcara mcara force-pushed the fix-colsel-type-conversion branch 2 times, most recently from 952dbe7 to 18d461d Compare March 26, 2021 16:44
@pllim
Copy link
Member

pllim commented Mar 26, 2021

Alas, unrelated samp failure is blocking the CI. If you approve #11411 , I can merge it and then it won't be a problem anymore.

@mcara mcara force-pushed the fix-colsel-type-conversion branch from 97d7c49 to b879e83 Compare March 27, 2021 01:42
@hamogu
Copy link
Member

hamogu commented Mar 28, 2021

The readthedocs failure might be related to the changes in CHANGES.rst or maybe not. I can't tell. However, it's certainly not related to the change that this PR is actually about (the C code in WCSLIB), thus I'm approving this.

@nden
Copy link
Contributor

nden commented Mar 28, 2021

@pllim Should we wait for Towncrier before merging this PR?

@mcara mcara force-pushed the fix-colsel-type-conversion branch from b6fb69c to 4520915 Compare March 28, 2021 15:20
@pllim
Copy link
Member

pllim commented Mar 29, 2021

Should we wait for Towncrier before merging this PR?

Yeah, let's wait a bit... The week(s) around release is always crazy...

@pllim
Copy link
Member

pllim commented Mar 29, 2021

The CI failure seems unrelated but something I'll need to investigate. I think upstream people like to release things on weekends... Ugh...

@mcara mcara force-pushed the fix-colsel-type-conversion branch from 4520915 to 2e69b52 Compare April 26, 2021 21:41
@mcara mcara force-pushed the fix-colsel-type-conversion branch from 51c6e38 to 44075af Compare April 26, 2021 22:57
@mcara mcara merged commit 348ddc4 into astropy:main Apr 27, 2021
@mcara mcara mentioned this pull request Jun 3, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hamogu hamogu hamogu approved these changes

@nden nden Awaiting requested review from nden

Assignees

@mcara mcara

Labels
Bug wcs
Projects
None yet
Milestone
v4.0.6
Development

Successfully merging this pull request may close these issues.

WCS generation parameter "colsel" does not seem to work
5 participants
@mcara @nden @pep8speaks @pllim @hamogu