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

Restrict u.spectral equivalency to contexts where it is required #673

Merged
merged 3 commits into from
May 11, 2020

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented May 7, 2020

Fix for #671

Fairly pedestrian implementation; @mhvk your proposed solution to use setup_class/teardown_class environments would require creating classes for the tests in question, right?

Leaving this for consideration now; perhaps some discussion is due if the tests now still test what they are supposed to test (i.e. would a user in a normal work environment expect those equivalencies to be in effect as well)?

@dhomeier
Copy link
Contributor Author

dhomeier commented May 7, 2020

Concerning the failed doctests - need to think of a clean way to make this work in the docs. I am wondering if using equivalencies is really the best way to tackle the problem with lower/upper boundaries in the spectral ranges...

@dhomeier
Copy link
Contributor Author

dhomeier commented May 7, 2020

Fixing the code rather than the tests hopefully will hold now. Concerning the failed remote_data tests, I got 404 errors from das.sdss.org yesterday roughly 2 out of 3 times.

The local astropy tests seem to succeed now at least without any failures potentially related to astropy/astropy#10283.
@nmearl I've had a second look for any other equivalency uses that would depend on u.spectral set and might not be fully covered by the tests; u.is_equivalent checks for spectral_axis type quantities in fitmodels, extract_spectral_region and spectrum_mixin are already doing it the Right Way™*. Can you think of any uses of Unit.to() that might still depend on the equivalency?

*Rather, either one of the two right ways – had forgotten that equivalency can also be passed as an option; for the single operation uses this may be preferable to the with context.

@mhvk
Copy link
Contributor

mhvk commented May 7, 2020

@dhomeier - answering without looking: yes, setup_class and teardown_class require test classes, but there are also module-level equivalents.

@dhomeier
Copy link
Contributor Author

dhomeier commented May 7, 2020

@mhvk thanks; since there are only three instances which are also easily managed with the kwarg, I think there is no need for a major restructuring of the tests at this point.

@nmearl
Copy link
Contributor

nmearl commented May 11, 2020

I've had a second look for any other equivalency uses that would depend on u.spectral set and might not be fully covered by the tests; u.is_equivalent checks for spectral_axis type quantities in fitmodels, extract_spectral_region and spectrum_mixin are already doing it the Right Way™*. Can you think of any uses of Unit.to() that might still depend on the equivalency?

It seems you've covered all the bases here. The failures are due to inconsistencies with upstream currently (since we moved SpectralCoord into astropy proper).

I'm tempted to say that this is good as it stands, as it passes all my use cases currently.

@nmearl
Copy link
Contributor

nmearl commented May 11, 2020

Thanks @dhomeier!

@nmearl nmearl merged commit ec1016d into astropy:master May 11, 2020
@dhomeier dhomeier deleted the encapsulate_spectral_equivalency branch May 14, 2020 21:00
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.

3 participants