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

Piecewise-constant chromatic variations (CMX) #1777

Merged
merged 28 commits into from
Nov 27, 2024
Merged

Conversation

abhisrkckl
Copy link
Contributor

@abhisrkckl abhisrkckl commented Jun 3, 2024

This depends on #1616. So That PR needs to be merged first. --- DONE.

Partially implements #1661.

@abhisrkckl abhisrkckl changed the title Piecewise-constant chromatic variations (CMX) WIP: Piecewise-constant chromatic variations (CMX) Jun 4, 2024
@abhisrkckl abhisrkckl changed the title WIP: Piecewise-constant chromatic variations (CMX) Piecewise-constant chromatic variations (CMX) Nov 13, 2024
@abhisrkckl abhisrkckl added the awaiting review This PR needs someone to review it so it can be merged label Nov 13, 2024
@kerrm
Copy link
Contributor

kerrm commented Nov 14, 2024

This looks like a lot of work!
Overall it looks fine to me.
I think the big thing is to add to the test an explicit verification that the chromatic calculation is correct. You've got some TOAs at different freqs, so verify that
(1) the delay is correct given the amplitude and chromatic index
(2) that the delay does not affect TOAs outside of any ranges
(3) that the lookup of ranges is correct, so do (1) for multiple events

@mtlam
Copy link
Member

mtlam commented Nov 20, 2024

I took a look and agree with @kerrm, the base functionality looks great. After any potential additional testing components might be added beyond the recent commits, I think it would be great to get this functionality in.

@abhisrkckl
Copy link
Contributor Author

I think the big thing is to add to the test an explicit verification that the chromatic calculation is correct. You've got some TOAs at different freqs, so verify that
(1) the delay is correct given the amplitude and chromatic index
(2) that the delay does not affect TOAs outside of any ranges
(3) that the lookup of ranges is correct, so do (1) for multiple events

Added tests for this

@abhisrkckl
Copy link
Contributor Author

I think this can be merged.

@dlakaplan dlakaplan merged commit f1f719b into nanograv:master Nov 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review This PR needs someone to review it so it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants