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

Clipping Map plots to a percentile interval #3100

Merged
merged 3 commits into from May 15, 2019

Conversation

Projects
None yet
5 participants
@ayshih
Copy link
Contributor

commented May 9, 2019

This PR adds the capability to easily clip Map plots to a percentile interval. Facilitates fixing of #2590. Discuss.

No clipping (the default)

>>> import sunpy.data.sample
>>> from sunpy.map import Map
>>> m = Map(sunpy.data.sample.AIA_171_IMAGE)
>>> m.peek()

a

With clipping

Here, the clipping percentile interval is (1%, 99.95%). That is, the lowest 1% and the highest 0.05% of pixels are being clipped out.

>>> import astropy.units as u
>>> import sunpy.data.sample
>>> from sunpy.map import Map
>>> m = Map(sunpy.data.sample.AIA_171_IMAGE)
>>> m.peek(clip_interval=(1., 99.95)*u.percent)

b

@ayshih ayshih added the map label May 9, 2019

@ayshih ayshih requested review from ehsteve and wafels May 9, 2019

@sunpy-bot

This comment was marked as outdated.

Copy link

commented May 9, 2019

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

1 similar comment
@sunpy-bot

This comment has been minimized.

Copy link

commented May 9, 2019

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

@nabobalis nabobalis added this to the 1.0 milestone May 10, 2019

@nabobalis

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

A changelog would be nice. Test wise, we could create a figure test?

Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
Show resolved Hide resolved sunpy/map/mapbase.py Outdated
@Cadair

This comment has been minimized.

Copy link
Member

commented May 10, 2019

I think this is a great little feature. I think we should probably edit the examples and docs to use it where appropriate with our sample data?

@ayshih ayshih force-pushed the ayshih:clip branch from b6dc0b2 to a911e9b May 10, 2019

@Cadair

Cadair approved these changes May 15, 2019

@Cadair Cadair added the [Review] label May 15, 2019

@Cadair Cadair merged commit cc355e1 into sunpy:master May 15, 2019

16 checks passed

ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 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
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/patch 90% of diff hit (target 89.96%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +0.03% compared to 8aa10a3
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190510.7 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details
title : `bool`, optional
If `True`, include the title.
clip_interval : two-element `~astropy.units.Quantity`, optional

This comment has been minimized.

Copy link
@samaloney

samaloney May 15, 2019

Contributor

Does this really need to be as astropy quantity? If so and the quantity is known should probably check for it raise and error if doesn't match?

This comment has been minimized.

Copy link
@ayshih

ayshih May 15, 2019

Author Contributor

The u.quantity_input decorator takes care of (some of) the error checking.

It may be slightly cumbersome for this to be a Quantity, but it's our guidelines to use units as much as possible. As a case in point, for this PR I played around with some of the underlying Matplotlib machinery, and I was initially confused because I didn't know whether I was supposed to be supplying percentages or fractions as inputs, so I had to empirically figure it out.

This comment has been minimized.

Copy link
@Cadair

Cadair May 15, 2019

Member

We are generally pretty stringent with everything being quantities where that makes sense. the @u.quantity_input will validate that the unit is u.percent

This comment has been minimized.

Copy link
@ehsteve

ehsteve May 15, 2019

Member

Thumbs up on using units here though the percent quantity is not so useful beyond just identifying itself as such.

This comment has been minimized.

Copy link
@ayshih

ayshih May 15, 2019

Author Contributor

Note that one could choose to specify clip_interval=(0.01, 0.9995)*u.one in the example in the first post and get the same clipping interval (1% to 99.95%)

This comment has been minimized.

Copy link
@samaloney

samaloney May 15, 2019

Contributor

Ok, I just really, really dislike the extra typing in cases like this.

This comment has been minimized.

Copy link
@Cadair

Cadair May 15, 2019

Member

heh:

>>> (0.99*u.one).to(u.percent)                                                                                                                                                                                                 
<Quantity 99. %>

Didn't know that worked...

@ayshih ayshih deleted the ayshih:clip branch May 21, 2019

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.