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

feat: add option to sigma-clip sub-bands homogeneously #927

Merged
merged 20 commits into from
Feb 13, 2024

Conversation

steven-murray
Copy link
Contributor

This adds several options to a new sigma_clip() function, including clip_type which specifies whether data should be directly clipped or accumulated over some axis before assessing the threshold. A placeholder for allowing the expected variance to be passed has also been added, but not piped through (i.e. you can't specify this on the CLI yet).

The reason that the expected variance can't be piped through easily is that there's no obvious way of getting the auto spectra that works for both redundantly averaged and non-averaged data input.

This adds several options to a new `sigma_clip()` function, including
``clip_type`` which specifies whether data should be directly clipped
or accumulated over some axis before assessing the threshold. A placeholder
for allowing the expected variance to be passed has also been added, but
not piped through (i.e. you can't specify this on the CLI yet).
@steven-murray steven-murray requested a review from jsdillon January 9, 2024 14:36
Copy link
Member

@jsdillon jsdillon left a comment

Choose a reason for hiding this comment

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

Mostly minor comments, though I think think it'd be nice to add a unit test for the new modes.

Also, a bunch of unit tests are failing... so that's fun.

hera_cal/lstbin_simple.py Outdated Show resolved Hide resolved
hera_cal/lstbin_simple.py Show resolved Hide resolved
hera_cal/lstbin_simple.py Show resolved Hide resolved
hera_cal/lstbin_simple.py Outdated Show resolved Hide resolved
hera_cal/lstbin_simple.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b045b6e) 97.16% compared to head (1cac6ce) 97.15%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #927      +/-   ##
==========================================
- Coverage   97.16%   97.15%   -0.01%     
==========================================
  Files          23       23              
  Lines       10638    10692      +54     
==========================================
+ Hits        10336    10388      +52     
- Misses        302      304       +2     
Flag Coverage Δ
unittests 97.15% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@jsdillon jsdillon left a comment

Choose a reason for hiding this comment

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

Just a couple of questions/comments, but nearly good to go!

hera_cal/lstbin_simple.py Outdated Show resolved Hide resolved
@@ -60,6 +60,8 @@

kwargs['output_flagged'] = not kwargs.pop('no_flagged_mode')
kwargs['output_inpainted'] = kwargs.pop("do_inpaint_mode")
kwargs['sigma_clip_subbands'] = [int(b) for b in kwargs["sigma_clip_subbands"].split(",")]
Copy link
Member

Choose a reason for hiding this comment

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

Has this been added to the argparser?

Also, this raises an interesting question about the best way to specify subbands from the commandline and/or from a TOML.

I'm using this format, which is in MHz. It's similar to something Aaron EW did for H4C, though I think he used channels. It's a lot more human readable, but it's a bit clunky:

https://github.com/HERA-Team/hera_pipelines/blob/46a6018bf497d1066fd242aaf01fa6e0a2aaa9fd/pipelines/h6c/idr2/v2/post-lstbin/h6c_pspec.toml#L15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's been added to the argparser. The format I'm using is like:

--sigma-clip-subbands 0,150,1000

This assumes that every channel belongs to a subband, so you just specify the edges. I could make it more general, using the tilde notation, but I'm not perfectly sure it's necessary in this case?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to use the tilde notation. It's more standard and it's not necessarily true that every channel belongs to a subband (e.g. FM).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've done this now.

@steven-murray steven-murray mentioned this pull request Feb 8, 2024
@jsdillon jsdillon self-requested a review February 13, 2024 19:13
Copy link
Member

@jsdillon jsdillon left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @steven-murray !

@jsdillon jsdillon merged commit be00ed9 into main Feb 13, 2024
9 of 10 checks passed
@jsdillon jsdillon deleted the more-general-sigma-clip branch February 13, 2024 19:14
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.

2 participants