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

PropagationBuffer #68

Merged
merged 29 commits into from
Jun 15, 2024
Merged

PropagationBuffer #68

merged 29 commits into from
Jun 15, 2024

Conversation

LuEdRaMo
Copy link
Collaborator

No description provided.

@LuEdRaMo LuEdRaMo marked this pull request as ready for review June 11, 2024 22:08
@lbenet
Copy link
Collaborator

lbenet commented Jun 12, 2024

Does the last commit mean that this PR is ready for review?

@LuEdRaMo
Copy link
Collaborator Author

Does the last commit mean that this PR is ready for review?

Indeed, this PR is ready for review.

Copy link
Collaborator

@lbenet lbenet left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for this valuable addition.
I left few comments, mostly related to adding comments or docstrings. There are I think two suggestions related to adding a new method, and (perhaps?) using broadcasting somewhere. Also, could you perhaps include some very naive performance comparison of "before" and "after" this PR?
None of the above suggestions are required, but would be welcome. So go ahead and merge when you think this is ok. I am approving this PR.

src/orbitdetermination/gaussinitcond.jl Show resolved Hide resolved
src/orbitdetermination/gaussinitcond.jl Show resolved Hide resolved
src/orbitdetermination/gaussinitcond.jl Show resolved Hide resolved
src/orbitdetermination/gaussinitcond.jl Outdated Show resolved Hide resolved
- `scalings::Vector{T}`: jet transport scaling factors.
- `jacobian::Matrix{T}`: residuals space to barycentric coordinates jacobian.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are the scaling factors somehow hidden in jacobian? (Or is it confussion due to the preceeding name?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

jacobian is a generalization of scalings. The former reduces to the latter when a TaylorN takes the form x + dx. Otherwise, for a nontrivial JT tail, jacobian is the full projection of the covariance matrix from residuals to barycentric space.

src/propagation/asteroid_dynamical_models.jl Outdated Show resolved Hide resolved
src/propagation/jetcoeffs.jl Outdated Show resolved Hide resolved
@LuEdRaMo
Copy link
Collaborator Author

Since tests pass, I'll proceed to merge...

@LuEdRaMo LuEdRaMo merged commit 8f1a664 into main Jun 15, 2024
6 of 8 checks passed
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