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

Generalize the RHE Function beyond AIA #231

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

GillySpace27
Copy link
Contributor

PR Description

@GillySpace27
Copy link
Contributor Author

This PR is associated with Issue #233 addressing the ability for the algorithm to operate on non-AIA datasets.

@@ -221,7 +225,6 @@ def nrgf(
width_function=np.std,
width_function_kwargs=None,
application_radius=1 * u.R_sun,
progress=True,
Copy link
Contributor

@nabobalis nabobalis Sep 23, 2024

Choose a reason for hiding this comment

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

These progress changes will need to be undone.

sunkit_image/radial.py Outdated Show resolved Hide resolved
sunkit_image/radial.py Outdated Show resolved Hide resolved
sunkit_image/radial.py Outdated Show resolved Hide resolved
sunkit_image/radial.py Outdated Show resolved Hide resolved
sunkit_image/radial.py Outdated Show resolved Hide resolved
sunkit_image/radial.py Outdated Show resolved Hide resolved
@nabobalis
Copy link
Contributor

Thanks for the PR @GillySpace27, what other data have were you trying to run this on?

We will need to get some test data to confirm it works for non-AIA data.

@GillySpace27
Copy link
Contributor Author

@nabobalis Thanks! The first non-AIA dataset I applied it to is some synthetic PUNCH test data. I'm interested in getting some others working too, including LASCO, SUVI, etc.

sunkit_image/radial.py Outdated Show resolved Hide resolved
new_map.plot_settings["norm"] = None

# Return the new SunPy map with RHEF applied
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Return the new SunPy map with RHEF applied


# Adjust plot settings to remove extra normalization
# This must be done whenever one is adjusting
# the overall statistical distribution of values

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

data[here] = ranking_func(smap.data[here])
if upsilon is not None:
data[here] = apply_upsilon(data[here], upsilon)
new_map = sunpy.map.Map(data, meta, autoalign=True)

# Final update to the map
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Final update to the map


# Perform the filtering operation
ranking_func = _select_rank_method(method)
# Apply ranking function
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Apply ranking function

sunkit_image/radial.py Outdated Show resolved Hide resolved
)

# Allocate storage for the filtered data
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Allocate storage for the filtered data

sunkit_image/radial.py Outdated Show resolved Hide resolved
@@ -695,46 +684,57 @@ def rhef(
https://www.proquest.com/docview/2759080511
"""

# Get the radii for every pixel
map_r = find_pixel_radii(smap).to(u.R_sun)
# Get the radii for every pixel, ensuring units are correct (in terms of pixels or solar radii)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Get the radii for every pixel, ensuring units are correct (in terms of pixels or solar radii)

Defaults to True.
upsilon : float or None, optional
A double-sided gamma function to apply to modify the equalized histograms. Defaults to 0.35.
method : str, optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a list of all the known methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

inplace, numpy, and scipy. They are defined above in a helper function. This functionality could probably be removed, but it is nice to have the flexibility I think.

method : str, optional
Method used to rank the pixels for equalization. Defaults to 'inplace'.
vignette : `astropy.units.Quantity`, optional
Radius beyond which pixels will be set to black. Defaults to ``1.5*u.R_sun``. Set to None to disable vignetting.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Radius beyond which pixels will be set to black. Defaults to ``1.5*u.R_sun``. Set to None to disable vignetting.
Radius beyond which pixels will be set to black. Defaults to None, must be in units that are compatible with "R_sun" as the value will be transformed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking that setting to NAN is probably more useful since the values aren't valid out there anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure but you won't pass nan to the keyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure - are you saying that the default should be a NAN value?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I was just confused.

@nabobalis
Copy link
Contributor

@nabobalis Thanks! The first non-AIA dataset I applied it to is some synthetic PUNCH test data. I'm interested in getting some others working too, including LASCO, SUVI, etc.

Is it possible we can add a file from the synthetic PUNCH test data to the repo?

sunkit_image/radial.py Outdated Show resolved Hide resolved
@GillySpace27 GillySpace27 marked this pull request as ready for review September 24, 2024 03:28
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.

2 participants