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

Add tests for sunpy.physics.differential_rotate #4333

Merged
merged 7 commits into from
Jul 25, 2020

Conversation

dipanshu231099
Copy link
Contributor

Description

Previously the tests for differential rotation were only considering a new observer.
I have created tests for differential rotations as the time parameter is altered.

Fixes #3239

@dipanshu231099 dipanshu231099 requested a review from a team as a code owner July 3, 2020 16:52
@nabobalis nabobalis added this to the 2.1 milestone Jul 4, 2020
@nabobalis nabobalis added No Changelog Entry Needed physics Affects the physics submodule Tests Affects tests in some measure labels Jul 4, 2020
@nabobalis nabobalis requested a review from wtbarnes July 4, 2020 07:52
@dipanshu231099
Copy link
Contributor Author

@nabobalis is it mergeable now ?

@nabobalis
Copy link
Contributor

This needs two reviews from people who have worked on sunpy.physics.differential_rotate before it can be merged.

@wtbarnes
Copy link
Member

wtbarnes commented Jul 6, 2020

So I know that this persisted prior to this PR, but it would be nice to break these tests up a bit. Rather than having one monolithic test_differential_rotate function, I'd prefer that we have a set of tests for this functionality.

As for the bits that test the time input, I think this could probably be parametrized, e.g. for a single function, the two possible input sets for observer and time could be ((new_observer, None), (None, new_time)).

@dipanshu231099
Copy link
Contributor Author

So I know that this persisted prior to this PR, but it would be nice to break these tests up a bit. Rather than having one monolithic test_differential_rotate function, I'd prefer that we have a set of tests for this functionality.

As for the bits that test the time input, I think this could probably be parametrized, e.g. for a single function, the two possible input sets for observer and time could be ((new_observer, None), (None, new_time)).

shall i make a different test definition for time tests then ? @wtbarnes

@nabobalis
Copy link
Contributor

Yes.

@wtbarnes
Copy link
Member

wtbarnes commented Jul 7, 2020

So I know that this persisted prior to this PR, but it would be nice to break these tests up a bit. Rather than having one monolithic test_differential_rotate function, I'd prefer that we have a set of tests for this functionality.
As for the bits that test the time input, I think this could probably be parametrized, e.g. for a single function, the two possible input sets for observer and time could be ((new_observer, None), (None, new_time)).

shall i make a different test definition for time tests then ? @wtbarnes

Yes that'd be great! If you're interested in taking a stab at seeing how the existing test code could be broken up, that could make things a bit nicer as well, e.g. having separate tests for all on disk, partially on disk, all of disk.

@pep8speaks
Copy link

pep8speaks commented Jul 8, 2020

Hello @dipanshu231099! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2020-07-25 07:52:47 UTC

@dipanshu231099
Copy link
Contributor Author

@wtbarnes i have segregated the different test blocks pre existing ones as well as new ones for the differential_rotate function

@dipanshu231099
Copy link
Contributor Author

@wtbarnes any other thing required in it?

@nabobalis nabobalis requested a review from wafels July 15, 2020 16:45
@nabobalis nabobalis removed request for a team and wafels July 25, 2020 07:46
@nabobalis nabobalis merged commit d8889ae into sunpy:master Jul 25, 2020
@nabobalis
Copy link
Contributor

Thanks @dipanshu231099

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Entry Needed physics Affects the physics submodule Tests Affects tests in some measure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need test for time input in differential rotation function
4 participants