add .cmap and .norm property to plot_settings

[sashank27]

Description

Replaces up plot_settings in sunpy.map.mapbase, and adds up .cmap and .norm so as to provide for a cleaner and usable API.

Fixes #3415
Fixes #3713

[ghost]

Thanks for the pull request @sashank27! Everything looks great!

[pep8speaks]

Hello @sashank27! 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-02-12 18:08:38 UTC

[sashank27]

@dstansby @Cadair @nabobalis please review this

[nabobalis]

I'll review it later this week but it seems you have broken some of the tests with the init changes:

>           return smap._new_instance(out_data, out_meta)
E           TypeError: _new_instance() missing 2 required positional arguments: 'cmap' and 'norm'

The other test fails seem to be fixed in master, so you might want to merge in master.

[wtbarnes]

Thanks for working on this @sashank27! I've left quite a few comments, but many of them are duplicates and quick changes. The biggest change is the logic around plot_settings in __init__.

The composite map and mapsequence code also have some plot_settings logic that needs to be addressed. The former is what is causing the doc build failure.

[wtbarnes]

After thinking about it a bit more, I think the property-setter approach for plot_settings is not the right way to proceed. It just overcomplicates things. I've added several notes where appropriate. I know this goes back on what I said before and I'm sorry for the confusion. Thanks for your patience! 😁

Also, I noticed that you removed all your changes to the examples? I think the examples should reflect the preferred way of setting the norm and cmap which is now through the respective properties rather than plot_settings. If you'd rather not include those changes here, we can always offload that to a separate issue/PR.

[sashank27]

@wtbarnes I was thinking to update all the examples on a separate PR, as this already has a lot of changes

[wtbarnes]

A few very minor changes

[wtbarnes]

@wtbarnes I was thinking to update all the examples on a separate PR, as this already has a lot of changes

That's completely fine, but it looks like (at least) one of the examples is causing the docs build to fail. Could you try to build the examples locally and fix up the ones that fail? Others can be left to separate PR.

[sashank27]

@wtbarnes I was thinking to update all the examples on a separate PR, as this already has a lot of changes

That's completely fine, but it looks like (at least) one of the examples is causing the docs build to fail. Could you try to build the examples locally and fix up the ones that fail? Others can be left to separate PR.

Well, previously this was the issue. I had to update all the examples to make the build pass.
I've decided now to update the examples here only.

[wtbarnes]

Ok so the docs build is failing because one of the examples is constructing the color map name incorrectly. The reason is because the observatory name (SDO in this case) is being excluded when creating the new header.

If you pass the additional keyword argument observatory=map_aia.observatory when creating the new header here:

sunpy/examples/sunpy_other_packages/reprojection_different_observers.py

Line 130 in eaab336

mars_header = sunpy.map.make_fitswcs_header(

I think this should fix the problem. Previously, this was not raising an exception because we were not validating the colormap when creating the Map and it was later being overridden by the old plot_settings dict which had the correct colormap name.

[wtbarnes]

Everything looks good to me, but something weird is happening with one of the examples and is causing the docs build to fail. For some reason, it is trying to smash together "stereo a" and "aia 171" as the color map name and then failing because that is not a valid color map name (obviously 😆 )

[wtbarnes]

Thanks @sashank27. One more approval and we'll be good to go.

I'm a bit concerned by why that change in the ordering of the maps was needed...Have the filenames changed that caused the ordering to be swapped? Or perhaps there's something strange going on in the factory? Either way, it is unrelated to this PR.

[Cadair]

Thanks so much for all your work on this @sashank27

I am sorry to have a few more things to change, but this is a pretty major change to one of the older bits of the code.

[ayshih]

I worry that there are additional use cases for plot_settings, beyond colormap and normalization, that will no longer be supported. Being able to store the plot settings with each map means that a user could more seamlessly work with a collection(/list/sequence) of maps, including slicing, without having to keep and synchronize a separate collection of plot settings.

However, during the dev meeting, we didn't see any plot settings would conceivably be used in this way by a user, aside from maybe interpolation.

[Cadair]

hey @sashank27

Thank you so much for working on this. We chatted about it today at the community meeting, and generally we all struggled to remember why we wanted to make this change in the first place, and we wanted to stop and think about it in terms of the future changes to Map (i.e. integration with ndcube).

Therefore we are going to put this on the agenda for the SunPy Coordination meeting at the end of March, and talk about it with all the other things relating to Map.

Sorry that we don't feel like we can just accept this as it is, the code is great and thank you very much for all your effort on it.

[nabobalis]

With the decision in #3415 (comment) I will be closing this PR as I believe the new change probably needs to be done from scratch (especially with all the file conflicts).

I want to thank you @sashank27 for the effort you put into this PR and I am sorry it was never merged due to us being undecided on the new formal API for this change.