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

Better algorithm for TCB -> TDB conversion #1765

Merged
merged 77 commits into from
May 22, 2024

Conversation

abhisrkckl
Copy link
Contributor

@abhisrkckl abhisrkckl commented May 14, 2024

Converts most parameters, including some that were excluded before.

Parameters that are not converted are marked explicitly.

Also includes some sourcery refactors.

@abhisrkckl
Copy link
Contributor Author

Are you going to update this as discussed to change the default behavior? Or will that be a future PR?

Done.

@dlakaplan
Copy link
Contributor

It looks like you put the conversion factor into a lot of unrelated tests. Does that actually make a difference? If the tests don't do TCB/TDB conversion, is there any step that would notice if the conversion factor is present or not?

@abhisrkckl
Copy link
Contributor Author

Since we made the conversion factor mandatory, I had to include them in tests whenever Parameter object was getting created. Otherwise the yest will fail.

@dlakaplan
Copy link
Contributor

OK, I guess I thought that the factor need not be mandatory but that the conversion step would fail without it.

@abhisrkckl
Copy link
Contributor Author

OK, I guess I thought that the factor need not be mandatory but that the conversion step would fail without it.

I think that will be more confusing. If we are going to throw an error for missing scaling factors, I'd prefer it to be thrown when the parameter is added, rather than at a later stage. This will also force the devs to think more carefully about the parameters, IMO.

@dlakaplan
Copy link
Contributor

OK. I can probably merge this, unless you want to present tomorrow first.

@abhisrkckl
Copy link
Contributor Author

OK... Please merge this. I got all the necessary feedback last week.

@dlakaplan dlakaplan merged commit b0fcc44 into nanograv:master May 22, 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.

None yet

2 participants