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

exp: propagation-centric host ckf #761

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

niermann999
Copy link
Contributor

@niermann999 niermann999 commented Nov 3, 2024

This is just a test to see what would need to be modified, in order to keep the navigation state alive and use candaidate caching in the navigator. So far, it only holds the 256 byte navigation state in addition to the bound track parameters and not the entire propagation state. This means, however, that the track parameters need to be copied in and out of the propagation state at every step and for every branch. This ckf implementation needs some additional changes in detray before it will compile.

Additionally, it sorts the valid measurements at every step, so that the best matching measurements get assign to the original branch and any new branches, in case the number of branches is limited to e.g. one or two.

The memory handling and the way the measurements are traces is not particularly smart, as that was not the focus of this example PR

@niermann999 niermann999 changed the title feat: propagation-centric host ckf exp: propagation-centric host ckf Nov 3, 2024
@niermann999 niermann999 force-pushed the ref-meas-range branch 3 times, most recently from da5a9e4 to 69d4b39 Compare November 4, 2024 16:44
Copy link

sonarqubecloud bot commented Nov 4, 2024

@beomki-yeo
Copy link
Contributor

I would be onboard with the idea of candidate caching, but this PR seems to be mixture of several independent changes. It would be good to split this PR

And I suggest to modify find_tracks.hpp instead of making find_trakcs_alt.hpp

@niermann999
Copy link
Contributor Author

I would be onboard with the idea of candidate caching, but this PR seems to be mixture of several independent changes. It would be good to split this PR

And I suggest to modify find_tracks.hpp instead of making find_trakcs_alt.hpp

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator? If I should port something from this PR into find_tracks.hpp I can do that of course (e.g. the measurement range finding?). Some of these changes actually belong to #714 and #763

@beomki-yeo
Copy link
Contributor

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator?

What do you mean? so are you going to add find_tracks_alt.hpp anyway?

@niermann999
Copy link
Contributor Author

I was actually planning to keep find_tracks.hpp as it is and instead equip it with a different navigator?

What do you mean? so are you going to add find_tracks_alt.hpp anyway?

That was my intention, yes. I would like to compare, if the additional memory consumption by the caching is possibly eating up the benefit from the more efficient navigation. But in order to make that comparison, we need to remove the additional work for the navigation cache from find_tracks.hpp. Does that make sense?

@beomki-yeo
Copy link
Contributor

No. I don't think that justifies your intention. you can compare the performance between main branch and your PR. Continuous benchmark was developed in the same spirit.

But in order to make that comparison, we need to remove the additional work for the navigation cache from find_tracks.hpp

What is the additional work and the navigation cache from find_tracks.hpp?

@niermann999
Copy link
Contributor Author

What is the additional work and the navigation cache from find_tracks.hpp?

It needs to hold more memory to cache candidates that it never uses (per branch) and we have seen that memory consumption makes a difference in performance. It also needs to build the cache and that means an additional step of sorting the candidate array every time a sensitive surface is reached. It also needs to resolve the trust levels and different update schemes of the cache (e.g. it rebuilds the entire cache if the next candidate is not reachable). That means a layer of of if-else constructs. All of this complexity is not needed in find_tracks.hpp and might slow it down, leading to an incorrect comparison imo

@beomki-yeo
Copy link
Contributor

beomki-yeo commented Nov 7, 2024

If I understood correctly, you want to compare the performance find_tracks.hpp and find_tracks_alt.hpp and to do this in correct way you also need to fix find_tracks.hpp?

May I suggest to separate this into two sequential steps rather doing this in one-go?

  1. Update find_tracks.hpp for the fair comparison (corresponding to your last comment) and see if performance improves
  2. Update with candidate caching and see if performance improves

@beomki-yeo
Copy link
Contributor

Then we don't have to make two parallel implementations for find_tracks.hpp.

@beomki-yeo
Copy link
Contributor

What is the status of this PR? If you agree with my argument, can we close this?

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