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

fix standard error of the mean #8826

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

benjaminpope
Copy link

@benjaminpope benjaminpope commented Sep 24, 2024

This PR addresses a bug in the fork of ImPlaneIA that constitutes calwebb_ami3.

Tasks

  • Review:
  • add a build milestone, i.e. Build 11.3 (use the latest build if not sure)
  • Does this PR change user-facing code / API? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • update or add relevant tests
    • update relevant docstrings and / or docs/ page
    • start a regression test and include a link to the running job (click here for instructions)
      • Do truth files need to be updated ("okified")?
        • after the reviewer has approved these changes, run okify_regtests to update the truth files
  • if a JIRA ticket exists, make sure it is resolved properly
news fragment change types...
- ``changes/<PR#>.general.rst``: infrastructure or miscellaneous change
- ``changes/<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``

## 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#>.ramp_fitting.rst``
- ``changes/<PR#>.gain_scale.rst``

## stage 2
- ``changes/<PR#>.assign_wcs.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``

@benjaminpope benjaminpope requested a review from a team as a code owner September 24, 2024 01:13
@github-actions github-actions bot added the ami label Sep 24, 2024
…andard deviation, when estimating errors for interferometric observables.
@benjaminpope
Copy link
Author

Re tests - there is not a current unit test for oifits.py. Testing this would require making a test dataset available in the repo. Do let me know your preferred course of action.

Copy link

@anand0xff anand0xff left a comment

Choose a reason for hiding this comment

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

I'm good with sigma-clipped means... [email protected] should be an owner perhaps, maybe instead of me?
Stats are being changed to use the standard error of the mean (by rcooper@).

@anand0xff
Copy link

Re test outfits files - we have to resolve how we write in covariances of cvs, triple products, and the closure quads (all complex or 2d observable) into outfits files. The standard is vaguely defined (or I don't understand it)

@benjaminpope
Copy link
Author

Do you need anything on the tests in this PR / from me? I've tested the updated uncertainties locally and they work fine, but best practice would have this be a full unit test.

@melanieclarke melanieclarke added this to the Build 11.2 milestone Sep 25, 2024
@rcooper295
Copy link
Contributor

Hi Ben, thanks for putting this in! We have gone back and forth on using the standard error of the mean vs. standard deviation a few times and at some point settled on taking the (sigma-clipped) median & standard deviation over the integrations, but I'm in the midst of updating the code to calculate covariances for various observables for each [baseline/triangle/quad] and use those for the error bars. You can see those changes here, though I'm still getting everything working. I can certainly change the default to use the mean rather than median if that's the consensus but I'm not sure if I should still be dividing those by sqrt(n_integrations)?

@benjaminpope
Copy link
Author

Hi Rachel,

If we're distilling a whole bunch of integrations to a single mean and uncertainty, the right mean to choose is the SEM (ie 1/sqrt(N) not the standard deviation. This closely matches eg AMICAL.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.86%. Comparing base (947d48b) to head (4cb314b).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
jwst/ami/oifits.py 57.14% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8826   +/-   ##
=======================================
  Coverage   61.86%   61.86%           
=======================================
  Files         377      377           
  Lines       38911    38912    +1     
=======================================
+ Hits        24071    24072    +1     
  Misses      14840    14840           

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

@rcooper295
Copy link
Contributor

Thanks Ben, I've updated the code to default to take the mean rather than median, and to take the SEM rather than just sqrt([co]variance). Hopefully this will address the issues raised here. Draft PR is here: #8846

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants