-
-
Notifications
You must be signed in to change notification settings - Fork 592
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
Move sunpy.cm to sunpy.visualization.colormaps #3410
Conversation
Thanks for the pull request @Cadair! Everything looks great! |
Hello @Cadair! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-10-30 18:01:46 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Also update
docs/dev_guide/sunpy_stability.yaml
- I suggest preserving the
show_colormaps()
figure in the code reference or docstring forsunpy.visualization.colormaps
, unless there's a good reason not to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
One more thing: it's beyond the scope of this PR, but it's fairly opaque how to use this package. The colormaps themselves are held in a list (cmlist
), but this fact isn't documented anywhere. Are we intentionally avoiding providing access like sunpy.visualization.colormaps.sdoaia171
?
So another fun thing, we define an This is pretty much fine with the deprecated version, but what now doesn't work is this:
|
Ha ha, of course, So, |
This might actually be a good time to change it.
Are we happy with this, because it did work before this change? |
Ok, with a little bit of high grade hackery, I fixed the |
The figure-test build is still broken, though... |
Breaking on raising the DepWarn. |
Yeah, the open question during the dev meeting was why the figure-test build encounters that problem when collecting tests, but not, for example, the 32-bit build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't be merged until the issue with the figure-test build is resolved
# Import the two original functions in this namespace and use __all__ to keep | ||
# import * behaviour the same. | ||
from sunpy.visualization.colormaps.cm import * | ||
|
||
__all__ = ['show_colormaps', 'cmlist'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, you're changing the behavior of from sunpy.cm import *
, but that's because it didn't previously have an __all__
. (For example, cmname
would get imported.) However, the previous behavior is essentially a bug, and no one would have relied on it, so I support this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is only cname.
This __all__
is copied from sunpy.cm.cm.__all__
which we were importing with import *
Co-Authored-By: Jack Ireland <jackireland@gmail.com>
Can someone else please review this and then merge it |
@@ -1,6 +1,3 @@ | |||
cm: | |||
status: mature | |||
comments: Only forwards compatible changes expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆
Fixes #3392