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

Allows for a color function to be specified to plot_field_lines() #70

Merged
merged 13 commits into from
Aug 18, 2021

Conversation

jeffreypaul15
Copy link
Collaborator

@jeffreypaul15 jeffreypaul15 commented Aug 9, 2021

Fixes #50
@dstansby I do have one question about how we want to deal with the functionality, in #50 you mentioned that the color function would return a string but gave me an example with a function that returns rgba values as a tuple.
I've accounted for that as pyvista also does account for either a string or a list of rgb values.
However, the a from rgba or opacity isn't really accounted for.
I figured instead of doing

color.pop()
# we could do
opacity = color.pop()

and set the default opacity to 1.

I've also made a few changes to account for the passing of lists to field_arrays while saving and loading.

  • Tests
  • Example

sunkit_pyvista/plotter.py Outdated Show resolved Hide resolved
@dstansby
Copy link
Member

dstansby commented Aug 9, 2021

@jeffreypaul15
Copy link
Collaborator Author

I agree that saving strings as rgba values makes more sense.
My question is do we have to worry about alpha or do we just set opacity as 1 for everything?

jeffreypaul15 and others added 2 commits August 9, 2021 22:45
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
@dstansby
Copy link
Member

dstansby commented Aug 9, 2021

If it's possible we should worry about alpha and set the opacity value appropriately.

@jeffreypaul15
Copy link
Collaborator Author

Yeah, I'll make that change

@dstansby dstansby added this to the 0.1 milestone Aug 13, 2021
@jeffreypaul15 jeffreypaul15 marked this pull request as draft August 13, 2021 18:30
@jeffreypaul15
Copy link
Collaborator Author

Requires #73 to be merged

@jeffreypaul15
Copy link
Collaborator Author

@dstansby Do we need to add an example for this?

@dstansby
Copy link
Member

Yes, it would be good to add an example. Just copying and modifying the current PFSS example to include a custom color function should be good.

@jeffreypaul15 jeffreypaul15 marked this pull request as ready for review August 17, 2021 03:34
@jeffreypaul15 jeffreypaul15 requested a review from dstansby August 17, 2021 03:34
# We plot the field lines
plotter.plot_field_lines(field_lines)

# We cal also specify a color function while plotting the field lines.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We cal also specify a color function while plotting the field lines.
# We can also specify a color function while plotting the field lines.

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 also add a bit more description of what this function needs to look like? Something like

This function takes a single field line, and returns a color. In this case we use a Matplotlib norm and colormap to return a tuple of RGBA values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing!

sunkit_pyvista/plotter.py Show resolved Hide resolved
@@ -131,7 +134,7 @@ def test_multi_block(plotter):
assert plotter.all_meshes['solar_axis'][0].n_points == 101


def test_field_lines(aia171_test_map, plotter):
def test_field_lines_and_color_func(aia171_test_map, plotter):
Copy link
Member

Choose a reason for hiding this comment

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

Is this a figure test? If not can we add a figure test with the custom color function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was going to suggest that, do we have a new figure test for this one? Or modify the existing figure to use colorfunc instead of the default colours

Copy link
Member

Choose a reason for hiding this comment

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

Lets modify the existing one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I've done that

@jeffreypaul15 jeffreypaul15 added the Feature Request New feature wanted. label Aug 17, 2021
@jeffreypaul15 jeffreypaul15 requested a review from dstansby August 17, 2021 16:13
@dstansby dstansby merged commit f6b2260 into sunpy:main Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature wanted.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow users to specify custom function to color field lines
2 participants