Include prism bandpasses in the roman module #1307

arunkannawadi · 2024-07-31T03:23:25Z

Addresses #1018

arunkannawadi · 2024-07-31T04:11:38Z

There's a lot of chatter about setuptools removing the test command. How do we want to fix the breakage?

https://setuptools.pypa.io/en/stable/history.html

rmjarvis · 2024-07-31T17:03:09Z

Eugh. I guess pin setuptools to <72 for now. We use it to run the C++ tests. I guess we'll need to come up with some other builder/runner for them.

rmjarvis · 2024-07-31T17:12:33Z

BTW, are you planning to actually do something to implement prism observations? This PR doesn't do anything except allow those bands to be selected. I think that's a bad idea if we don't also do something to make them simulate the prism dispersion physics.

arunkannawadi · 2024-07-31T17:41:57Z

I figured that somebody would be confused because the changes in the PR doesn't do justice to the title :-)

Troxel and I have generated prism images along with Roman SN PIT with these changes here, some in skyCatalogs here using the dispersive photon operator here. All of these come together in the roman_imsim package here.

I'd still like to have this merged by v2.6 since someone at STScI is going to take this over from us and I'd like them to be able to use GalSim+skyCatalogs without needing any modifications. Apart from having a similar photon operator here in the unit tests, do you have anything in mind? I am not sure if this would be meaningful at all.

rmjarvis · 2024-07-31T17:54:50Z

I feel like some kind of sanity check is warranted here. I'm worried that someone who isn't using the roman_imsim code will just try to make an image with one of these as the filter, and then be upset that the image doesn't actually have any dispersion. I don't know what the best place for it is though.

Maybe just add a logger warning along the lines of "Warning: selecting a dispersive bandpass. This should be accompanied by a suitable photon operator (e.g. from roman_imsim) to actually implement the dispersive effect. This compatibility is not checked."

@rmandelb @matroxel What do you think?

matroxel · 2024-07-31T18:04:56Z

We need to at least get the right thermal backgrounds for these bandpasses and an implementation of the zodiacal light that works before merging into main I'd think?

A warning would be good, and we should probably include an example dispersion photon op and some updated documentation in demo13 or a new demo?

arunkannawadi · 2024-07-31T18:13:27Z

Do we have the thermal backgrounds from the project? For other short wavelength bands, they are unavailable and set to 0 here, so I followed the same. We have nominal zodical backgrounds through those bandpasses here although they may not be precise enough for prism.

Warning + demo with dispersion photon operator sounds good. If we have to switch the backgrounds, then I'd suggest we keep the issue open but merge this PR with warning+demo.

matroxel · 2024-07-31T18:20:21Z

The blue bands round to zero at some decimal place. The wide prism/grism bandpasses won't. This is something we'll have to ask about, its not on the Roman page that I found.

matroxel · 2024-07-31T18:23:48Z

I guess we should be properly accounting for it vs wavelength like we will for the zodiacal light for the W filter and prism/grism. W filter is also not well represented right now.

arunkannawadi · 2024-07-31T18:52:30Z

For some context, Roman SN PIT folks just want to be able to test their spectral extraction code from a reasonable-looking image, not necessarily realistic. Depending on when v2.6 is aimed for, getting realistic backgrounds may be out of scope. For this initial PR, my minimal goal is to be able to get the bandpasses by calling galsim.roman.getBandpasses(), which is not possible at present.

.github/workflows/ci.yml

.github/workflows/wheels.yml

rmjarvis

I think this PR is currently waiting on some kind of warning to let users know that selecting the Prism doesn't automatically give them Prism physics. Probably pointing to the implementation in roman_imsim that does.

AFAIU, the other items mentioned above (thermal backgrounds zodiacal light, etc.) for the Prism/Grism are considered out of scope for this PR and would be implemented in a later one. But please correct me if we want something better than just 0.0 for those now.

arunkannawadi · 2024-08-14T19:36:33Z

I changed the PR title to reflect the code changes.

For the warnings, I could think of two ways - either include an optional argument to getBandpasses that will decide whether to fetch non-imaging bands and warn the user if they are fetched or modify the __getitem__ method for the bandpass_dict that will warn if the keys corresponding to non-imaging bands are passed. I kinda like the second one, but I don't know if this would annoy users who might be used to just iterating over all the bands.

rmjarvis · 2024-08-14T19:45:11Z

Here's an idea, which might be better anyway. Add an option to getBandpasses like include_nonimaging_bands=True, which would include the Prism and Grism, which defaults to False. And maybe then it would be sufficient to just put a warning in the doc string about how that isn't sufficient to get prism physics and where the user might go to get it. At least then, it requires the user to be more intentional about selecting those bands, so maybe they would see the documentation about it.

arunkannawadi

Do we have at least a ball park value for the backgrounds in these bands? I can add a note in the docstring appropriately.

arunkannawadi · 2024-08-15T00:56:14Z

tests/test_roman.py

@@ -322,7 +322,7 @@ def test_roman_backgrounds():
    # The routine should not allow us to look directly at the sun since the background there is high
    # (to understate the problem).  If no date is supplied, then the routine assumes RA=dec=0 means
    # we are looking at the sun.
-    bp_dict = galsim.roman.getBandpasses()
+    bp_dict = galsim.roman.getBandpasses(include_all_bands=True)


I know you normally like to keep the old tests as they were, but I think this should be fine?

This change shouldn't be necessary. However, you should add a test that the Prism/Grism bands aren't included yet. Then the test you added below accounts for the include_all_bands=True case where you check that they are included.

rmjarvis · 2024-08-15T03:41:38Z

tests/test_roman.py

+
+    assert 'Grism_0thOrder' in bp_all
+    assert 'Grism_1stOrder' in bp_all
+    assert 'SNPrism' in bp_all


Or do it here, adding the converse asserts that these are not in bp_imaging.

rmjarvis · 2024-08-16T19:04:38Z

I'm happy with this now. @matroxel or @rmandelb did either of you want to take a look?

rmjarvis · 2024-08-16T19:05:27Z

@arunkannawadi Looks like you might need to rebase it to the current main. GitHub is complaining.

arunkannawadi · 2024-08-16T19:23:28Z

galsim/roman/roman_bandpass.py

@@ -134,8 +139,7 @@ def getBandpasses(AB_zeropoint=True, default_thin_trunc=True, **kwargs):
    bandpass_dict = {}
    # Loop over the bands.
    for index, bp_name in enumerate(data.dtype.names[1:]):
-        # Need to skip the prism and grism (not used for weak lensing imaging).
-        if bp_name=='SNPrism' or bp_name=='Grism_1stOrder' or bp_name=='Grism_0thOrder':
+        if include_all_bands is False and bp_name in ('Grism_0thOrder', 'Grism_1stOrder', 'SNPrism'):


Do we want to save this list somewhere (and reuse them in the tests may be) instead of hardcoding them here?

I could imagine this going in galsim/roman/__init__.py as nonimaging_bands or something.

…odule Co-authored-by: Michael Troxel <matroxel@gmail.com>

that non-imaging bands are NOT included when include_all_bands is not passed.

arunkannawadi added the roman Relevant to Roman Space Telescope specifically, especially including the galsim.roman module. label Jul 31, 2024

arunkannawadi added this to the v2.6 milestone Jul 31, 2024

arunkannawadi marked this pull request as ready for review July 31, 2024 03:57

arunkannawadi linked an issue Jul 31, 2024 that may be closed by this pull request

Simulate prism observations #1018

Open

arunkannawadi requested a review from rmjarvis July 31, 2024 04:10

arunkannawadi removed this from the v2.6 milestone Jul 31, 2024

arunkannawadi force-pushed the #1018 branch from 62d00f9 to c1b5b22 Compare July 31, 2024 17:53

rmjarvis reviewed Jul 31, 2024

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

rmjarvis reviewed Jul 31, 2024

View reviewed changes

.github/workflows/wheels.yml Outdated Show resolved Hide resolved

rmjarvis requested changes Aug 14, 2024

View reviewed changes

arunkannawadi changed the title ~~Simulate prism observations~~ Include prism bandpasses in the roman module Aug 14, 2024

arunkannawadi force-pushed the #1018 branch from 22442d4 to c1bca00 Compare August 15, 2024 00:43

arunkannawadi removed a link to an issue Aug 15, 2024

Simulate prism observations #1018

Open

arunkannawadi requested a review from rmjarvis August 15, 2024 00:55

arunkannawadi commented Aug 15, 2024

View reviewed changes

rmjarvis reviewed Aug 15, 2024

View reviewed changes

rmjarvis approved these changes Aug 16, 2024

View reviewed changes

arunkannawadi force-pushed the #1018 branch 3 times, most recently from 7cd906d to 230be6b Compare August 16, 2024 19:20

arunkannawadi commented Aug 16, 2024

View reviewed changes

Michael Troxel and others added 11 commits August 16, 2024 17:49

Modifications to enable using the roman prism bandpass in the roman m…

0c74ae3

…odule Co-authored-by: Michael Troxel <matroxel@gmail.com>

Add thermal backgrounds for dispersive elements

2866e1b

Add Grism to short wavelength bandpasses

a19c52b

Stop special casing dispersive elements

dcfed9a

Add dispersive bands in test_roman

9c7244a

Add an optional argument to include all Roman bands

6522b8c

Include all Roman bands in background test

dcd37bf

Add a smoke test for non-imaging Roman bands

b93f018

Add converse asserts

2646e0b

that non-imaging bands are NOT included when include_all_bands is not passed.

Document that thermal background is set to zero

283cf8f

Define and use non_imaging_bands

6f4ba8b

arunkannawadi force-pushed the #1018 branch from 230be6b to 6f4ba8b Compare August 16, 2024 21:50

rmjarvis added this to the v2.6 milestone Aug 20, 2024

rmjarvis merged commit dcb134c into main Sep 11, 2024
10 checks passed

rmjarvis deleted the #1018 branch September 11, 2024 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include prism bandpasses in the roman module #1307

Include prism bandpasses in the roman module #1307

arunkannawadi commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024 •

edited

Loading

rmjarvis commented Jul 31, 2024

rmjarvis commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024

rmjarvis commented Jul 31, 2024 •

edited

Loading

matroxel commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024

matroxel commented Jul 31, 2024 •

edited

Loading

matroxel commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024 •

edited

Loading

rmjarvis left a comment

arunkannawadi commented Aug 14, 2024

rmjarvis commented Aug 14, 2024

arunkannawadi left a comment

arunkannawadi Aug 15, 2024

rmjarvis Aug 15, 2024 •

edited

Loading

rmjarvis Aug 15, 2024

rmjarvis commented Aug 16, 2024

rmjarvis commented Aug 16, 2024

arunkannawadi Aug 16, 2024

arunkannawadi Aug 16, 2024

rmjarvis Aug 16, 2024

Include prism bandpasses in the roman module #1307

Include prism bandpasses in the roman module #1307

Conversation

arunkannawadi commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024 • edited Loading

rmjarvis commented Jul 31, 2024

rmjarvis commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024

rmjarvis commented Jul 31, 2024 • edited Loading

matroxel commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024

matroxel commented Jul 31, 2024 • edited Loading

matroxel commented Jul 31, 2024

arunkannawadi commented Jul 31, 2024 • edited Loading

rmjarvis left a comment

Choose a reason for hiding this comment

arunkannawadi commented Aug 14, 2024

rmjarvis commented Aug 14, 2024

arunkannawadi left a comment

Choose a reason for hiding this comment

arunkannawadi Aug 15, 2024

Choose a reason for hiding this comment

rmjarvis Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

rmjarvis Aug 15, 2024

Choose a reason for hiding this comment

rmjarvis commented Aug 16, 2024

rmjarvis commented Aug 16, 2024

arunkannawadi Aug 16, 2024

Choose a reason for hiding this comment

arunkannawadi Aug 16, 2024

Choose a reason for hiding this comment

rmjarvis Aug 16, 2024

Choose a reason for hiding this comment

arunkannawadi commented Jul 31, 2024 •

edited

Loading

rmjarvis commented Jul 31, 2024 •

edited

Loading

matroxel commented Jul 31, 2024 •

edited

Loading

arunkannawadi commented Jul 31, 2024 •

edited

Loading

rmjarvis Aug 15, 2024 •

edited

Loading