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

LASCO_tutorials_and_masking #6576

Merged
merged 3 commits into from Apr 23, 2023
Merged

LASCO_tutorials_and_masking #6576

merged 3 commits into from Apr 23, 2023

Conversation

mwhv2
Copy link
Contributor

@mwhv2 mwhv2 commented Nov 8, 2022

This pull request fixes #6420 and fixes #3098. The tutorial lasco_overlay.py demonstrates the process required to overlay an AIA image onto a processed LASCO C2 image. It doubles as an example to show the usage of hvpy, since the Helioviewer Client is deprecated (#6404).

The tutorial lasco_mask.py is partly based upon a script by @AlthKouloumvakos (in #3098) that shows how to create a custom mask for an unprocessed LASCO C2 image that is downloaded using Fido. Overall, this tutorial combines elements from the current two masking tutorials and could help serve as a guide for creating custom masks.

I have also added the process from the lasco_mask tutorial as a property in the LASCO subclass. This allows a user to generate a LACSO mask (e.g., using mask = lasco.mask_occulter) which they can then apply to their Map. The bounds I have set for the LASCO C2 mask is based on the specified field of view, as well as in comparison to LASCO C2 data in HelioViewer. C3 data uses the mask recommended in LASCO calibration directory (this way the occulter pylon is also masked). I have not found many examples of LASCO C1 data, so I have not added a mask for this detector (but I would be happy to add one).

@dstansby
Copy link
Member

@mwhv2 are you looking for comments on this? Just wanted to check since it's in draft state

@mwhv2
Copy link
Contributor Author

mwhv2 commented Nov 17, 2022

@dstansby I was correcting some of the CI issues before marking it ready. Otherwise, yes!

Below are screenshots of the plots produced in the two tutorials.
aia_lasco_c2
lasco_mask

@mwhv2 mwhv2 marked this pull request as ready for review November 17, 2022 18:45
@mwhv2 mwhv2 requested review from a team as code owners November 17, 2022 18:45
Cadair
Cadair previously requested changes Nov 21, 2022
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @mwhv2 the examples look excellent.
The only issues I have are with the mask calculation method.

sunpy/map/sources/soho.py Outdated Show resolved Hide resolved
sunpy/map/sources/soho.py Outdated Show resolved Hide resolved
@dstansby dstansby requested a review from Cadair December 14, 2022 14:56
@dstansby dstansby added Documentation Affects the documentation backport 4.0 labels Dec 14, 2022
@dstansby dstansby added the Needs Review Needs reviews before merge label Jan 4, 2023
@nabobalis nabobalis added this to the 5.0.0 milestone Jan 31, 2023
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
@nabobalis
Copy link
Contributor

Sorry it took so long for a review @mwhv2

@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Feb 2, 2023
@nabobalis
Copy link
Contributor

Can you merge in main please? Hopefully that will fix the failing checks.

@nabobalis nabobalis removed the request for review from Cadair February 4, 2023 22:05
@nabobalis nabobalis added the Needs Review Needs reviews before merge label Feb 4, 2023
@nabobalis
Copy link
Contributor

Thanks for the fixes @mwhv2, I will try to get others to review this soon.

@nabobalis nabobalis added the Examples Affects the Example Gallery label Feb 8, 2023
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Apart from explaining assume_spherical_screen the lasco overlay example looks fine. I've left some comments on the other example that would make the code simpler and easier to read.

examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/plotting/lasco_overlay.py Outdated Show resolved Hide resolved
@nabobalis
Copy link
Contributor

I rebased the PR and fixed hopefully the last few review comments.

@nabobalis nabobalis removed this from the 5.0.0 milestone Mar 24, 2023
@nabobalis nabobalis added this to the 5.0.0 milestone Apr 18, 2023
@nabobalis nabobalis added the Needs Review Needs reviews before merge label Apr 19, 2023
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

One more request to improve variable names

examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
examples/map/lasco_mask.py Outdated Show resolved Hide resolved
@nabobalis nabobalis requested review from dstansby and removed request for a team April 19, 2023 21:20
mwhv2 and others added 3 commits April 19, 2023 16:43
Adds two LASCO tutorials to the example gallery. mask_occulter creates masks for LASCO C2 and C3 maps.
Co-authored-by: David Stansby <dstansby@gmail.com>
@nabobalis nabobalis removed the Needs Review Needs reviews before merge label Apr 19, 2023
@nabobalis
Copy link
Contributor

nabobalis commented Apr 19, 2023

I rebased just to make sure the CI is passing, considering how on fire it has been recently.

@dstansby dstansby added the No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers) label Apr 23, 2023
@dstansby dstansby merged commit c4e52f7 into sunpy:main Apr 23, 2023
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Affects the documentation Examples Affects the Example Gallery No Backport A PR that isn't to be backported to any release branch. (To be used as a flag to other maintainers)
Projects
None yet
4 participants