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.

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

Initial implementation for validating two WCS #433

Merged
merged 11 commits into from
Jul 7, 2021

Conversation

adwaitbhope
Copy link
Contributor

Description

Fixes #428.

An initial implementation of the validate function for checking if two WCS objects are compatible with each other to reproject an NDCube on top of another.

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

logic looks fine. Couple of small comments and needs tests 😄

ndcube/utils/wcs.py Outdated Show resolved Hide resolved
ndcube/utils/wcs.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Jun 22, 2021

Hello @adwaitbhope! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers!

Comment last updated at 2021-07-07 16:44:45 UTC

@adwaitbhope
Copy link
Contributor Author

I've added test cases for this function, but for the positive test case, we have the same object as source and target. Should we introduce another WCS in the fixtures?

ndcube/tests/test_utils_wcs.py Outdated Show resolved Hide resolved
ndcube/tests/test_utils_wcs.py Outdated Show resolved Hide resolved
@DanRyanIrish
Copy link
Member

Should we introduce another WCS in the fixtures?

I don't think it's necessary at this stage. The test as is adequately tests the current implementation. If/when this function moves to a more complicated implementation, adapting the test should be part of the work. Adding a new fixture would be part of that.

ndcube/utils/wcs.py Outdated Show resolved Hide resolved
@Cadair Cadair added this to the 2.0 milestone Jul 7, 2021
@Cadair Cadair merged commit 577b7b2 into sunpy:main Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate WCS Function
4 participants