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

Parallel RFI filtering #35

Merged
merged 3 commits into from
Sep 6, 2022
Merged

Parallel RFI filtering #35

merged 3 commits into from
Sep 6, 2022

Conversation

ljgray
Copy link
Contributor

@ljgray ljgray commented Aug 16, 2022

In its current implementation, the RFI Filter step of the daily pipeline takes ~25 minutes. Most of this time is spent taking a rolling median, and it is done in a single process. This PR should address the issue by pre-calculating a median across frequencies to flatten the auto vis. data, then distributing across the frequency axis to distribute the computation across processes.

@ljgray ljgray requested review from jrs65 and tristpinsm and removed request for tristpinsm August 16, 2022 21:53
ch_util/tools.py Outdated Show resolved Hide resolved
ch_util/tools.py Outdated Show resolved Hide resolved
ch_util/tools.py Outdated Show resolved Hide resolved
ch_util/rfi.py Outdated Show resolved Hide resolved
ch_util/rfi.py Outdated Show resolved Hide resolved
ch_util/rfi.py Outdated Show resolved Hide resolved
@ljgray
Copy link
Contributor Author

ljgray commented Aug 16, 2022

Although the products aren't quite right yet, this reduces the RFIFilter step down to ~15 seconds in a daily pipeline run

@ljgray ljgray force-pushed the ljg/parallel-rfi branch 4 times, most recently from 01aa599 to bba1a5a Compare August 17, 2022 22:19
@ljgray
Copy link
Contributor Author

ljgray commented Aug 17, 2022

Depends on radiocosmology/caput#211

@ljgray ljgray force-pushed the ljg/parallel-rfi branch 5 times, most recently from 6b216ed to cb8b593 Compare August 17, 2022 23:43
@ljgray ljgray requested a review from jrs65 August 18, 2022 00:04
@ljgray ljgray marked this pull request as ready for review August 18, 2022 00:04
@ljgray
Copy link
Contributor Author

ljgray commented Aug 18, 2022

Interestingly, the mask being produced is different from that produced by the previous method.

@ljgray
Copy link
Contributor Author

ljgray commented Aug 19, 2022

@jrs65 this produces the same mask now as previous revisions, so it should be good to go unless there are additional changes.

jrs65
jrs65 previously approved these changes Sep 6, 2022
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.

Approved, but please quickly change the comment, and condense the commits (if it makes sense to do so), before merging.

ch_util/rfi.py Outdated
Comment on lines 598 to 599
limit_range : slice, optional
Data is limited to this range in the freqeuncy axis. Defaults to Ellipsis.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change the comment as the default is not actually an Ellipsis (even though it's effectively the same).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch, that's a remnant of a previous way I had tried implementing it

Running number_deviations across inputs causes the entire operation to
take place on a single process. This is modified to be distributed
across many nodes.
@ljgray ljgray merged commit b74fcd5 into master Sep 6, 2022
@ljgray ljgray deleted the ljg/parallel-rfi branch September 6, 2022 21:32
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