-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implementing the RHE Filter #205
Conversation
I added an example and it runs! I don't know what to do about the pre-commit tests though. |
You can run the pre-commit locally using tox, It seems to be complaining about the commented out test code which we can remove or uncomment out if its useful? |
Is there anything else that needs to be done for this pull request? The changelog maybe? |
I'll review it again but for now I think a changelog entry is still missing. |
Should I address the failing tests, or do you do that? |
The build_docs is failing to download the data from the VSO in a different example. There isn't anything we can do. |
I made another version of the example using functions and I wanted to see if it was better or not. I also added logging. Do y'all like either of those choices? It seems something might need to be changed to allow the images to appear in-line like before. I placed the functionalized version in a separate file for now. |
I am personally not a fan. I don't see the objective of using functions like that. |
Sorry for the lack of response. Thanks for the PR @GillySpace27 and thanks for putting up with the long review. |
Can you check once more that the figure test output looks correct @GillySpace27? |
The images at this link<https://output.circle-artifacts.com/output/job/3b424f3b-a39d-470c-8ea8-680bc2fe46c7/artifacts/0/.tmp/py312-figure/figure_test_images/fig_comparison.html> seem correct. Thanks!
From: Nabil Freij ***@***.***>
Date: Friday, July 19, 2024 at 11:10 AM
To: sunpy/sunkit-image ***@***.***>
Cc: Gilbert, Gilly ***@***.***>, Mention ***@***.***>
Subject: Re: [sunpy/sunkit-image] Implementing the RHE Filter (PR #205)
[EXTERNAL EMAIL]
Can you check once more that the figure test output looks correct @GillySpace27<https://urldefense.com/v3/__https:/github.com/GillySpace27__;!!D-JDmu3Lc2wo0Jiybg!eCXaTl94CsQp3OmwsnYAWfB98natmHItXR_hxzdaAdNuohmo6aCYBrKT0bJSAg4fLXbpuOMz8dZ7n76lswBu$>?
|
When will this show up in new versions of the package? I thought it would be there today but it doesn't seem to be. `(base) cgilbert@GillyMacbook ~ % conda_create_ARM sunkit python=3.9 ==> WARNING: A newer version of conda exists. <== Please update conda by running
Package Planenvironment location: /opt/homebrew/anaconda3/envs/sunkit added / updated specs: The following packages will be downloaded:
The following NEW packages will be INSTALLED: ca-certificates pkgs/main/osx-arm64::ca-certificates-2024.7.2-hca03da5_0 None Proceed ([y]/n)? Downloading and Extracting Packages To activate this environment, use$ conda activate sunkitTo deactivate an active environment, use$ conda deactivateRetrieving notices: ...working... done
|
My current plan is to wait for more pull requests to be reviewed and merged and then release v0.6 in a month or so. |
Ok, cool! Thanks! |
Sorry for the delay but they're a bunch of open PRs that I would like to get in before a new release as well as I hope the new coalignment API. |
PR Description
The Radial package has been updated to include the Radial Histogram Equalizing Filter as well as a double-sided gamma function that we call an Upsilon function that is utility that could be useful elsewhere.
Implementation follows chapter 3 of my thesis: https://www.proquest.com/docview/2759080511, as well as a journal article in prep (Gilly 2024).
This PR addresses the following issue: #204