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

New chp type to run some common sidereal processing tasks #161

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented Feb 10, 2023

This PR introduces a new chp processing type, which runs the barebones daily pipeline steps to produce a delay spectrum from a sidereal stream. The type is linked to an existing daily processing revision, so the available tags are pulled from the tags completed by the linked day. The linked daily revision is by default the most recent, but this can be changed in the revconfig file after creation.

The idea is that anyone using this type is going to want to be customizing the jobtemplate in order to do something different from the linked pipeline revision. This just provides the barebones template, which should reproduce the delay spectrum that was created by the daily revision, assuming the venv is using the same commits (this would have to be manually configured).

It also adds a chp type for generating the wavelet spectrum of a given day based on radiocosmology/draco#237

Copy link
Contributor

@sjforeman sjforeman left a comment

Choose a reason for hiding this comment

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

This looks good to me overall, although I won't approve it because Richard should probably have the final say.

A few comments:

  • Should we consider using the 82-day rev06 stack in BlendStack instead of the rev03 stack? Occasionally in rev06 validation, we've seen hints of artifacts (e.g. vertical lines in delay spectra) that may be coming from the rev03 stack instead of individual rev06 days themselves, and presumably the rev06 stack is cleaner overall.

  • Should we consider another level of abstraction for DelaySpectrumProcessing? Most of the code in that class isn't specific to the precise reprocessing we want to do. Thus, we could consider bundling that code into a generic template (called ReProcessing or something), and then have DelaySpectrumProcessing as a subclass of that, only specifying the modifications to default_params, default_script etc. In the future, if we need to some other partial reprocessing of a revision, we'd only need to subclass ReProcessing and write the corresponding script.

@ljgray
Copy link
Contributor Author

ljgray commented Feb 14, 2023

This looks good to me overall, although I won't approve it because Richard should probably have the final say.

Agreed

A few comments:

* Should we consider using the 82-day rev06 stack in BlendStack instead of the rev03 stack? Occasionally in rev06 validation, we've seen hints of artifacts (e.g. vertical lines in delay spectra) that may be coming from the rev03 stack instead of individual rev06 days themselves, and presumably the rev06 stack is cleaner overall.

This can definitely be the default, although that should perhaps be a change for the standard Daily config in rev_07 of the daily pipeline. For reprocessing daily:rev_06, I think we should continue to use the rev_03 stack so no additional changes are introduced other than the new static mask.

* Should we consider another level of abstraction for `DelaySpectrumProcessing`? Most of the code in that class isn't specific to the precise reprocessing we want to do. Thus, we could consider bundling that code into a generic template (called `ReProcessing` or something), and then have `DelaySpectrumProcessing` as a subclass of that, only specifying the modifications to `default_params`, `default_script` etc. In the future, if we need to some other partial reprocessing of a revision, we'd only need to subclass `ReProcessing` and write the corresponding script.

I like this idea, although I think we will have to put some thought into the implementation so that we don't end up with a bunch of different default_scripts. I'll make a quick change to this in a separate commit to experiment a bit.

@ljgray ljgray force-pushed the ljg/sidereal-mask-type branch 3 times, most recently from c9bba86 to c68d460 Compare February 15, 2023 19:28
@ljgray
Copy link
Contributor Author

ljgray commented Feb 15, 2023

I've also added a task to get the static rfi mask and broadcast to the shape of the incoming data, and updated the reprocessing config to reflect the thresholdvisweight refactor

@ljgray ljgray force-pushed the ljg/sidereal-mask-type branch 2 times, most recently from 26029c0 to d77d0c2 Compare February 15, 2023 21:30
@ljgray ljgray requested a review from hgan0 February 16, 2023 21:54
@ljgray ljgray force-pushed the ljg/sidereal-mask-type branch 4 times, most recently from 9b57519 to baf0b7b Compare February 24, 2023 22:04
Copy link
Contributor

@jrs65 jrs65 left a comment

Choose a reason for hiding this comment

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

This looks really nice. Even if we don't end up using it, having a ready-to-go implementation of the functionality will be great for when we do need to do something similar.

For instance I can imagine running Minori's bandpass averaging code over the processed days using all this which would be great.

ch_pipeline/analysis/flagging.py Outdated Show resolved Hide resolved
@ljgray ljgray force-pushed the ljg/sidereal-mask-type branch 2 times, most recently from 83cfab4 to f5211c0 Compare March 3, 2023 17:45
@ljgray ljgray requested a review from jrs65 March 3, 2023 17:53
ch_pipeline/analysis/flagging.py Outdated Show resolved Hide resolved
ch_pipeline/analysis/flagging.py Outdated Show resolved Hide resolved
ch_pipeline/analysis/flagging.py Outdated Show resolved Hide resolved
@ljgray ljgray requested a review from jrs65 March 3, 2023 20:21
@ljgray ljgray force-pushed the ljg/sidereal-mask-type branch 2 times, most recently from 2913069 to fc75b0e Compare March 8, 2023 22:03
Copy link
Contributor

@sjforeman sjforeman left a comment

Choose a reason for hiding this comment

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

Looks great! I had only one minor comment to add. All of my previous comments have been addressed, so I'll leave it to @jrs65 to provide a final approval if he's happy as well

ch_pipeline/analysis/flagging.py Outdated Show resolved Hide resolved
@ljgray ljgray changed the title New chp type to generate delay spectrum from a sidereal stream New chp type to run some common sidereal processing tasks Apr 18, 2023
This adds a `delay` processing type to the types available in chp. The
basic implementation of the new type loads a sidereal stream file, runs
the standard daily processing masking, and produces a delay spectrum and
high-pass filtered delay spectrum. A given revision of the `delay` type
is linked to an existing revision of the `daily` type, so available tags
are pulled from the completed tags of the linked `daily` revision.
@ljgray ljgray added the area/pipeline pipeline config/infrastructure label Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/pipeline pipeline config/infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants