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

Time derivatives of NE_SW (NE_SW1, ...) #1859

Merged
merged 13 commits into from
Nov 27, 2024
Merged

Conversation

abhisrkckl
Copy link
Contributor

Complements #1854

@abhisrkckl abhisrkckl added the awaiting review This PR needs someone to review it so it can be merged label Nov 11, 2024
@dlakaplan
Copy link
Contributor

Maybe clarify that you are putting in time derivatives here (and not partial derivatives needed for fitting)?

@abhisrkckl abhisrkckl changed the title Derivatives of NE_SW Time derivatives of NE_SW (NE_SW1, ...) Nov 12, 2024
@kerrm
Copy link
Contributor

kerrm commented Nov 14, 2024

Two quick comments:
(1) perhaps add a short circuit for non time dependent cases to avoid any performance regression
(2) the test case should include a test of the correctness, e.g. that NE_SW1 = 1 produces a value of 5 a year 0 and 10 at year 5

@abhisrkckl
Copy link
Contributor Author

Two quick comments:
(1) perhaps add a short circuit for non time dependent cases to avoid any performance regression
(2) the test case should include a test of the correctness, e.g. that NE_SW1 = 1 produces a value of 5 a year 0 and 10 at year 5

Added both.

@abhisrkckl
Copy link
Contributor Author

I think this is ready to merge.

@dlakaplan dlakaplan merged commit 86e5385 into nanograv:master Nov 27, 2024
7 checks passed
@abhisrkckl abhisrkckl deleted the ne_sw1 branch January 20, 2025 08:52
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.

3 participants