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

Miscellaneous fixes following initial science validation comments. #131

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

schlafly
Copy link
Collaborator

@schlafly schlafly commented Jul 3, 2024

This PR makes a few changes following RTB comments.

I have added an additional sentence describing how maggies are converted into photons to the bandpass page at readthedocs.

The romanisim truncation support was half-baked and didn't propagate through to the places it needed to; this has been fixed.

@schlafly schlafly requested a review from a team as a code owner July 3, 2024 20:16
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.94%. Comparing base (0a1aac5) to head (1b2ffb4).
Report is 63 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
+ Coverage   90.78%   90.94%   +0.15%     
==========================================
  Files          17       17              
  Lines        1671     1700      +29     
==========================================
+ Hits         1517     1546      +29     
  Misses        154      154              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@schlafly
Copy link
Collaborator Author

schlafly commented Jul 3, 2024

@AndreaBellini, I think this addresses your requested doc change.

@ojustino, thanks for noticing the issue with the read_pattern not being correctly propagated through in the truncated case. Please see if this addresses your issue.

Copy link
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@ojustino
Copy link

Hi @schlafly, no problem, and thanks for making these fixes. The read_pattern-related additions make sense to me, and a quick check with my validation code now shows truncation working as intended.

@schlafly
Copy link
Collaborator Author

Thanks Justin. @AndreaBellini, if the addition to the bandpass docs here
https://romanisim--131.org.readthedocs.build/en/131/romanisim/bandpass.html
addresses your concerns, I'll go ahead and merge this.

@AndreaBellini
Copy link

AndreaBellini commented Jul 11, 2024 via email

@schlafly
Copy link
Collaborator Author

Thanks Andrea. I've now added a section to the L1 page that has the conversion from photons to DN via the gain.
https://romanisim--131.org.readthedocs.build/en/131/romanisim/l1.html#gain

@AndreaBellini
Copy link

AndreaBellini commented Jul 11, 2024 via email

@schlafly
Copy link
Collaborator Author

We are currently treating the system as if a photon generates an electron. Yes, if there's an additional level in the chain from DN to photons, it's not presently included; can you point me to some documentation?

@schlafly
Copy link
Collaborator Author

This paper finds quantum yields peaking at 1.05 at 550 nm, with an average in the F062 band of probably around 1.03.
https://iopscience.iop.org/article/10.1088/1538-3873/ac46ba#paspac46baf7
In other bands they find ~1.003%. That's certainly an effect I'm neglecting, assuming a quantum yield of 1. The fact that the quantum yields are not exactly one means to me that there's some ~imprecision in the notion of gain and ramp fitting; when we pretend that the electrons are Poisson distributed, in fact they have some slightly more complex distribution. It doesn't seem to me that this is an effect that we should be simulating at this stage---we certainly don't have requirements about modeling quantum yields, etc..

Is this in fact the effect you're concerned about here?

@AndreaBellini
Copy link

AndreaBellini commented Jul 12, 2024 via email

@schlafly schlafly merged commit 719c32b into spacetelescope:main Jul 15, 2024
22 checks passed
@schlafly schlafly deleted the rtb-misc branch July 15, 2024 12:52
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.

4 participants