-
Notifications
You must be signed in to change notification settings - Fork 167
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
JP-3718: Improve AMI observable averaging, error calculations #8846
JP-3718: Improve AMI observable averaging, error calculations #8846
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8846 +/- ##
==========================================
+ Coverage 64.54% 64.56% +0.02%
==========================================
Files 375 375
Lines 38739 38890 +151
==========================================
+ Hits 25003 25109 +106
- Misses 13736 13781 +45 ☔ View full report in Codecov by Sentry. |
01b21a6
to
efdc204
Compare
02f0b1c
to
15d38f3
Compare
15d38f3
to
12807b2
Compare
Note: this PR requires spacetelescope/stdatamodels#357 to be merged first, then we will update the stdatamodels requirement in pyproject.toml here. |
f487469
to
a0bd5d0
Compare
6c42f01
to
4176c82
Compare
Initial regression tests started here, with stdatamodels branch from spacetelescope/stdatamodels#357: |
7e10b41
to
caea87a
Compare
1d84a4c
to
c91b184
Compare
ebe25cd
to
64b5483
Compare
Running regression tests again with stdatamodels on main: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm reviewing mostly for documentation and style, since you have confirmed that the regression test changes for AMI products are as expected. There are a few minor things to clean up, but I think this is looking good.
It looks like errors here are now standard error of the mean, addressing concerns in #8826. Can we close that PR in favor of this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this Rachel, and it looks excellent overall! In general the documentation is clear and detailed, and the code is structured well and easy to follow. Quite a few comments, but most of them are just small suggestions for clarity or docstrings that aren't quite done yet.
…les from already-averaged fringe phases or amplitudes, WIP
…an for uncertainty estimates
…documentation with examples.
… calibrator_object_id in calibrated files. Use ROLL_REF instead of PA_V3, though leaving old variable names for now.
…ition angle related keywords
64b5483
to
7745ad2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all my comments have been addressed - thanks for all the documentation updates! I found two tiny remaining typos, and I'd like to re-run regression tests just in case, but I will approve now.
|
||
.. code-block:: python | ||
|
||
# Create a F480M filter + Vega bandpass ASDF file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix! These instructions work for me now, to make the bandpass file and feed it to ami_analyze via --bandpass
.
@@ -81,20 +144,19 @@ Interferometric observables | |||
:Data model: `~jwst.datamodels.AmiOIModel` | |||
:File suffix: _ami-oi.fits, _amimulti-oi.fits | |||
|
|||
. The inteferometric observables are saved as OIFITS files, a registered FITS format | |||
The inteferometric observables are saved as OIFITS files, a registered FITS format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inteferometric observables are saved as OIFITS files, a registered FITS format | |
The interferometric observables are saved as OIFITS files, a registered FITS format |
Nov. 2024 email discussion with Tony Sohn, Paul Goudfrooij confirmed V2/V3 coordinate | ||
rotation back to "North up" equatorial orientation should use ROLL_REF + V3I_YANG | ||
(= PA_APER). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super tiny, but this section still needs to be un-indented, to match the "Notes" indent level.
Re-running regression tests again here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now too, pending regtests
Regtests show some diffs in NIRISS WFSS products, as well as the AMI, but it looks like that might be due to your CRDS delivery today, @rcooper295? Can you take a look at those and see if they are as expected, too? If so, we can merge this and okify all the NIRISS changes. |
Yes, I think the changes in the regtest outputs are still expected from the changes so it looks ok to me! |
Great! Thanks for checking. I will merge and okify. |
This PR addresses JP-3718, which changes how observables are averaged and their errors are calculated. Now calculating all "downstream" observables (i.e. calculated from fringe phases and amplitudes) from already-averaged fringe quantities, rather than calculating and then averaging. Now calculating covariances between related fringes and amplitudes (between baselines, triangles, and quads) and taking standard error of the mean (from covariance) as uncertainty.
Resolves JP-3718
Resolves #8733
Closes #8826
Tasks
Build 11.3
(use the latest build if not sure)no-changelog-entry-needed
)changes/
:echo "changed something" > changes/<PR#>.<changetype>.rst
(see below for change types)docs/
pageokify_regtests
to update the truth filesnews fragment change types...
changes/<PR#>.general.rst
: infrastructure or miscellaneous changechanges/<PR#>.docs.rst
changes/<PR#>.stpipe.rst
changes/<PR#>.datamodels.rst
changes/<PR#>.scripts.rst
changes/<PR#>.fits_generator.rst
changes/<PR#>.set_telescope_pointing.rst
changes/<PR#>.pipeline.rst
stage 1
changes/<PR#>.group_scale.rst
changes/<PR#>.dq_init.rst
changes/<PR#>.emicorr.rst
changes/<PR#>.saturation.rst
changes/<PR#>.ipc.rst
changes/<PR#>.firstframe.rst
changes/<PR#>.lastframe.rst
changes/<PR#>.reset.rst
changes/<PR#>.superbias.rst
changes/<PR#>.refpix.rst
changes/<PR#>.linearity.rst
changes/<PR#>.rscd.rst
changes/<PR#>.persistence.rst
changes/<PR#>.dark_current.rst
changes/<PR#>.charge_migration.rst
changes/<PR#>.jump.rst
changes/<PR#>.clean_flicker_noise.rst
changes/<PR#>.ramp_fitting.rst
changes/<PR#>.gain_scale.rst
stage 2
changes/<PR#>.assign_wcs.rst
changes/<PR#>.badpix_selfcal.rst
changes/<PR#>.msaflagopen.rst
changes/<PR#>.nsclean.rst
changes/<PR#>.imprint.rst
changes/<PR#>.background.rst
changes/<PR#>.extract_2d.rst
changes/<PR#>.master_background.rst
changes/<PR#>.wavecorr.rst
changes/<PR#>.srctype.rst
changes/<PR#>.straylight.rst
changes/<PR#>.wfss_contam.rst
changes/<PR#>.flatfield.rst
changes/<PR#>.fringe.rst
changes/<PR#>.pathloss.rst
changes/<PR#>.barshadow.rst
changes/<PR#>.photom.rst
changes/<PR#>.pixel_replace.rst
changes/<PR#>.resample_spec.rst
changes/<PR#>.residual_fringe.rst
changes/<PR#>.cube_build.rst
changes/<PR#>.extract_1d.rst
changes/<PR#>.resample.rst
stage 3
changes/<PR#>.assign_mtwcs.rst
changes/<PR#>.mrs_imatch.rst
changes/<PR#>.tweakreg.rst
changes/<PR#>.skymatch.rst
changes/<PR#>.exp_to_source.rst
changes/<PR#>.outlier_detection.rst
changes/<PR#>.tso_photometry.rst
changes/<PR#>.stack_refs.rst
changes/<PR#>.align_refs.rst
changes/<PR#>.klip.rst
changes/<PR#>.spectral_leak.rst
changes/<PR#>.source_catalog.rst
changes/<PR#>.combine_1d.rst
changes/<PR#>.ami.rst
other
changes/<PR#>.wfs_combine.rst
changes/<PR#>.white_light.rst
changes/<PR#>.cube_skymatch.rst
changes/<PR#>.engdb_tools.rst
changes/<PR#>.guider_cds.rst