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 for HighLevelWCS attribute error #349

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Fix for HighLevelWCS attribute error #349

merged 4 commits into from
Mar 13, 2023

Conversation

keflavich
Copy link
Contributor

This fixes #348, which is caused by a HighLevelWCSWrapper being passed where a LowLevelWCS object is expected.

@astrofrog I don't understand how this happens, as it's something created in a tall dask command tree by spectral-cube. I can't come up with a minimal example that reproduces this error.

I added test coverage, but none of these tests reproduce the error. They do, however, do add coverage by testing the _reproject_blocked code, which afaict was not covered at all (though I could easily have missed it)

@codecov
Copy link

codecov bot commented Mar 10, 2023 •
edited
Loading

Codecov Report

Merging #349 (fcd811e) into main (2145448) will decrease coverage by 0.09%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##             main     #349      +/-   ##
==========================================
- Coverage   92.44%   92.36%   -0.09%     
==========================================
  Files          24       24              
  Lines         821      825       +4     
==========================================
+ Hits          759      762       +3     
- Misses         62       63       +1
Impacted Files Coverage Δ
reproject/utils.py 84.27% <75.00%> (-0.34%) ⬇️
reproject/wcs_utils.py 96.00% <100.00%> (+0.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@keflavich keflavich mentioned this pull request Mar 10, 2023
Closed
@astrofrog
Copy link
Member

I think the best fix might be to actually make it so that has_celestial can take a non-FITS WCS high level WCS object - basically we should check in the else clause that if it is a high level WCS object we should extract .low_level_wcs before proceeding with the check.

@keflavich
Copy link
Contributor Author

I'm happy with that approach, I just wasn't sure it was intended. I'd already prepped a version that worked like that; I'll push it shortly

@keflavich
Copy link
Contributor Author

Ah, the weird thing is, what has_celestial is getting passed right now is something like HighLevelWCS(SlicedWCS(HighLevelWCS))), which isn't really right

astrofrog
astrofrog requested changes Mar 11, 2023
View reviewed changes
Copy link
Member

@astrofrog astrofrog 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 overall but just a couple of comments!

reproject/utils.py Outdated Show resolved Hide resolved
astrofrog
astrofrog reviewed Mar 11, 2023
View reviewed changes
reproject/utils.py Outdated Show resolved Hide resolved
astrofrog
astrofrog reviewed Mar 11, 2023
View reviewed changes
reproject/utils.py Outdated Show resolved Hide resolved
@keflavich
Copy link
Contributor Author

My spammed changes will need to be squashed of course

@keflavich
Copy link
Contributor Author

@astrofrog I'm going to revert the change you suggested; it is invalid. If we use wcs_out = <something>, it becomes a local variable in the scope of the function reproject_single_block, which isn't what we want.

@keflavich keflavich mentioned this pull request Mar 11, 2023
Merged
astrofrog
astrofrog reviewed Mar 11, 2023
View reviewed changes
Copy link
Member

@astrofrog astrofrog 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 now! Just a small comment below, and otherwise just need to fix the code style and squash commits. Thanks!

reproject/tests/test_high_level.py Outdated Show resolved Hide resolved
@keflavich
fix high level wcs wrapping bug and add a regression test
3fb80e6 
fix typo

fix bugs

might've fixed it?

try to add a test (it doesn't test what I want, but it adds _some_
coverage) and add a hack-fix

_actually_ expand test coverage; forgot to pass args on previously

black

black
@keflavich
remove some unnecessary lines, then add an example of what looks weird
78d5bc2 
in modding has_celestial

implement Tom's suggestions

fix

fix^2

the recursive approach might allow for infinite loops

fix the unbound local error

remove unnecessary test
@keflavich
add a test that fails before this change:
60baa56 
If this test code is injected at the start of reproject_single_block,
the error will occur
print(has_celestial(HighLevelWCSWrapper(SlicedLowLevelWCS(wcs_out, slices=slices))))

style fixes

style

black
@keflavich
Copy link
Contributor Author

One outstanding issue: the test_has_celestial code isn't actually testing what happens when it is given a HighLevelWCS with another HighLevelWCS as the .low_level_wcs attribute, because that can't happen - the initializers for HighLevelWCS don't allow it. The issue was actually that a HighLevelWCS was being passed as an argument to a function that assumed only a LowLevelWCS was being passed.

So... I'm going to remove that unnecessary code.

@astrofrog astrofrog merged commit 3bb79cd into astropy:main Mar 13, 2023
@astrofrog astrofrog added the bug label May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astrofrog astrofrog astrofrog requested changes

@Cadair Cadair Cadair approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Failure w/astropy-dev from spectral-cube: APE14 issue?
3 participants
@keflavich @astrofrog @Cadair