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

Changes to LASCO color tables #2731

Merged
merged 16 commits into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@wafels
Copy link
Member

commented Aug 16, 2018

The existing LASCO color tables don't work well with helioviewer JP2 images. The color tables are highly compressed with over half the colors simply being white, so the colors are crammed in to an unnecessarily small range of values. This PR replaces those color tables with matplotlib colormaps that make nice looking helioviewer JP2 images and reasonable looking images of Level 0.5 image data (bearing in mind that Level 0.5 data is not background subtracted). This PR fixes #2724 .

@pep8speaks

This comment has been minimized.

Copy link

commented Aug 16, 2018

Hello @wafels! Thanks for updating the PR.

Comment last updated on September 11, 2018 at 09:13 Hours UTC
@sunpy-bot

This comment has been minimized.

Copy link

commented Aug 16, 2018

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

@wafels wafels self-assigned this Aug 16, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

ooo a bold move!!

What's the justification for removing them rather than just making them not the default?

@Cadair Cadair added the map label Aug 16, 2018

@Cadair Cadair added this to the 1.0 milestone Aug 16, 2018

@wafels

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

I suppose we could keep them for historical reasons but they are really bad color tables. They are highly compressed, and in my opinion work against the desire to see features in the data. The new C2 color table looks superficially like the old C2 color table, so I expect few complaints about that. The C3 color table is quite a bit different, but you do see more detail. I think these are improvements to SunPy's ability to display LASCO images nicely. The only reason to keep them is for historical reasons.

@Cadair

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

I agree. imo it's probably worth keeping them in sunpy.cm just in case people want them for something specific and weird.


# Now create the color dictionary in the correct format
cdict = create_cdict(rgb[:, 0], rgb[:, 1], rgb[:, 2])
return colors.LinearSegmentedColormap('SOHO LASCO C{:s}'.format(str(number)), cdict)

This comment has been minimized.

Copy link
@Cadair

Cadair Aug 16, 2018

Member

These cms and the old ones have the same name.

Do we need this? Could we not just change the map defaults to the mpl names?

This comment has been minimized.

Copy link
@wafels

wafels Aug 16, 2018

Author Member

I did this for consistency with the names we give to the other solar physics derived color tables.

This comment has been minimized.

Copy link
@wafels

wafels Aug 16, 2018

Author Member

I'm not super happy with the name of the historical color table functions. Maybe we should write "instrument_team_lasco_color_table", or "sswidl_lasco_color_table" ?

This comment has been minimized.

Copy link
@ehsteve

ehsteve Aug 20, 2018

Member

I like ''sswidl_lasco_color_table''. I agree we should keep the old version just in case.

This comment has been minimized.

Copy link
@wafels

wafels Aug 22, 2018

Author Member

Ok.

@@ -130,7 +130,7 @@ def __init__(self, data, header, **kwargs):
self.meta['date_obs'] = self.meta['date-obs']
self._nickname = self.instrument + "-" + self.detector
self.plot_settings['cmap'] = plt.get_cmap('soholasco{det!s}'.format(det=self.detector[1]))
self.plot_settings['norm'] = ImageNormalize(stretch=source_stretch(self.meta, PowerStretch(0.5)))
self.plot_settings['norm'] = ImageNormalize(stretch=source_stretch(self.meta, LinearStretch()))

This comment has been minimized.

Copy link
@Cadair

Cadair Aug 16, 2018

Member

Is this also the best stretch for the actual calibrated lasco data?

This comment has been minimized.

Copy link
@wafels

wafels Aug 16, 2018

Author Member

Level 0.5 and Level 1 data don't look great with any amount of stretch since you really need to subtract a background model in order to see anything like a CME. Slightly better choices might be, say PowerStretch(0.5), but the advantage is not great.

@@ -11,7 +11,7 @@
from matplotlib import colors

from astropy.units import Quantity
from astropy.visualization import PowerStretch

This comment has been minimized.

Copy link
@ehsteve

ehsteve Aug 20, 2018

Member

Why are you importing LinearStretch if you then don't use it?

This comment has been minimized.

Copy link
@wafels

wafels Aug 22, 2018

Author Member

removed.

wafels added some commits Aug 22, 2018

@Cadair

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

I think you will have to make a change in cm/cm.py to register the legacy colormaps with matplotlib?

@@ -137,14 +143,14 @@ def get_cmap(name):
raise ValueError("Colormap {name!s} is not recognized".format(name=name))


def show_colormaps(filter=None):
def show_colormaps(search=None):

This comment has been minimized.

Copy link
@Cadair

Cadair Aug 23, 2018

Member

can you add a second changelog file under breaking for this please?

This comment has been minimized.

Copy link
@wafels

wafels Aug 23, 2018

Author Member

So I add a file called 2731.breaking.rst ?

This comment has been minimized.

Copy link
@Cadair
SSWIDL color scaling for LASCO level 0.5 and 1.0 is highly compressed
and does not display the data well."""
if number == 2:
rgb = cm.get_cmap('gist_heat')(np.linspace(0, 1, 256)) * 255

This comment has been minimized.

Copy link
@Cadair

Cadair Aug 23, 2018

Member

This seems... redundant?

The operation we want to do here is to give the same colourmap a different name. We can do that with a simple call to https://matplotlib.org/api/cm_api.html#matplotlib.cm.register_cmap

I think therefore my advice is to move the contents of this function directly into cm/cm.py and just call

mpl.cm.register_cmap("SOHO LASCO C2", mpl.cm.get_cmap("gist_heat"))

This comment has been minimized.

Copy link
@Cadair

Cadair Aug 23, 2018

Member

Also as an aside, we should probably not use import matplotlib.cm as cm in a sunpy submodule named cm where there is also a file called cm it get's very confusing very quick!!

@Cadair

Cadair approved these changes Aug 29, 2018

@ehsteve
Copy link
Member

left a comment

Looks good!

def lasco_color_table(number):
"""Returns one of the fundamental color tables for SOHO LASCO images."""
def sswidl_lasco_color_table(number):
"""Returns one of the previously used color tables for SOHO LASCO images.

This comment has been minimized.

Copy link
@ehsteve

ehsteve Aug 29, 2018

Member

Remove use of word "previous" as this will likely not mean much in a few years. Replace with SSW-defined tables.

wafels and others added some commits Aug 29, 2018

@Cadair Cadair force-pushed the wafels:fixlascocolortables branch from c9f989c to 5a56ef8 Aug 30, 2018

@Cadair Cadair requested a review from ehsteve Aug 30, 2018

@ehsteve

ehsteve approved these changes Sep 6, 2018

@Cadair Cadair merged commit 6e12a13 into sunpy:master Sep 12, 2018

9 checks passed

ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
codecov/patch 93.33% of diff hit (target 74.17%)
Details
codecov/project 85.36% (+11.18%) compared to 25bb804
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.