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

Changed saving of colors as strings to an rgb tuple #73

Merged
merged 7 commits into from
Aug 14, 2021

Conversation

jeffreypaul15
Copy link
Collaborator

@jeffreypaul15 jeffreypaul15 commented Aug 10, 2021

Fixes #76
Previously, field_arrays contained the name of the color as the 0th index of the color field array.
I've changed this to always have a tuple of type (r,g,b) as pyvista accepts this format for colors as well.

The default value of color is now always white - previously being nan. This avoids unnecessary checks when loading the colors.

@jeffreypaul15
Copy link
Collaborator Author

@dstansby Previously when a line was plotted, the arc_length legend was shown to denote the length in terms of solar radius, since we're using white as the default color, this is replaced with a white line. Do we need the previous functionality?
image
image

@dstansby
Copy link
Member

I think having the line as white is actually better 😄

changelog/73.trivial.rst Outdated Show resolved Hide resolved
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
@@ -218,9 +219,13 @@ def plot_map(self, m, clip_interval: u.percent = None, **kwargs):
else:
clim = [0, 1]
cmap = self._get_cmap(kwargs, m)
self.plotter.add_mesh(map_mesh, cmap=cmap, clim=clim, **kwargs)
color = kwargs.pop('color', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was wrong in my earlier comment - we shouldn't allow color as a keyword argument to plot_map, as it makes no sense to plot a map in a single color. Instead just leave this line in, add a warning below in the if block, and don't add color to the field array.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'll make this change.

color = kwargs.get('color', np.nan)
arrow_mesh.add_field_array([color], 'color')
self.plotter.add_mesh(arrow_mesh, **kwargs)
color = kwargs.pop('color', 'white')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create a new method called _extract_color that:

  • Takes kwargs as an input
  • Returns a color as an RGB tuple?

This was we can reduce code duplication in several of the new lines you've added.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this makes more sense I shall do that

Copy link
Collaborator Author

@jeffreypaul15 jeffreypaul15 Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I'll add this in.

@dstansby dstansby added this to the 0.1 milestone Aug 13, 2021
@dstansby dstansby merged commit 289f482 into sunpy:main Aug 14, 2021
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.

Save colormaps as 'cmap' in field arrays instead of 'color'
3 participants