Type unrolling jacobian transformation and covariance transport #295
Conversation
|
Unit tests for polar and cylindrical coordinate are added. Maybe I will stop editing this PR from now on.. |
|
@niermann999 Rebased |
| matrix_operator().template set_block( | ||
| jac_to_global, bound_to_free_rotation, e_free_pos0, e_bound_loc0); | ||
| // Set d(x,y,z)/d(loc0, loc1) | ||
| Derived<transform3_t>().set_bound_pos_to_free_pos_derivative( |
There was a problem hiding this comment.
So this actually calculated the local to global jacobian?
There was a problem hiding this comment.
Yes It is local to global (d(x,y,z)/d(loc0,loc1) means (loc0, loc1) -> (x,y,z) )
There was a problem hiding this comment.
I was a bit confused by the name set_bound_pos_to_free_pos_derivative, I think :P
| inline void set_unknown() { _status = navigation::status::e_unknown; } | ||
|
|
||
| DETRAY_HOST_DEVICE | ||
| inline void set_on_module() { |
There was a problem hiding this comment.
Where are these two functions used?
There was a problem hiding this comment.
In the path_correction.inl where I made a stupid actor ...
There was a problem hiding this comment.
Oh, I skipped that for now, because I don't have much time :P. So this runs after the navigator, but before some other actor (such as the stepper, I assume) that only does things when the state is on_module?
There was a problem hiding this comment.
This is an actor so it runs after the stepper and navigator.
It makes sure that track propagates the given path length (one full turn in path_correction.inl) until it reaches the targeted surface. All information from navigation.update() is ignored as the candidates are cleared for every actor call. This actor is only for testing purpose.
There was a problem hiding this comment.
Hm, I think this breaks encapsulation pretty badly. I am not entirely sure what this actor is trying to solve: it seems to try to bypass the navigator altogether? Does it try to insert a fake candidate to make the stepper take a certain step without re-navigating? If possible, I would prefer writing e.g. a dummy navigator that does nothing but use an externally provided intersection instead of modifying the existing one this way
|
@niermann999 Changes from #303 seem causing some errors GitLab CI Log
Running with gitlab-runner 15.0.0 (febb2a09) Skipping Git submodules setup You are in 'detached HEAD' state. You can look around, make experimental If you want to create a new branch to retain commits you create, you may git switch -c Or undo this operation with: git switch - Turn off this advice by setting config variable advice.detachedHead to false HEAD is now at 1028072 Small fixes
|
…d-jacobian-transformation
|
It's fixed after merging #314 |
| prop_state._heartbeat = true; | ||
| auto &navigation = prop_state._navigation; | ||
| auto &stepping = prop_state._stepping; | ||
| navigation.set_full_trust(); |
There was a problem hiding this comment.
This should not be possible, no actor should be able to raise the trust level arbitrarily
There was a problem hiding this comment.
My meaning is, this does not force the trust level to e_full_trust, if the stepper reduced the trust level first. It might not have the effect you seem to expect here
/// Update navigation trust level to full trust
DETRAY_HOST_DEVICE
inline void set_full_trust() {
_trust_level = _trust_level <= navigation::trust_level::e_full
? _trust_level
: navigation::trust_level::e_full;
}
This behaviour is very much intended.
There was a problem hiding this comment.
Come to think of it, this method should be removed from the navigator state altogether. It has literally has no effect...
| navigation.set_full_trust(); | ||
|
|
||
| scalar residual = actor_state._path - stepping.path_length(); | ||
| stepping.set_constraint(residual); |
There was a problem hiding this comment.
This only makes sense if residual is smaller than the distance to the next surface (I don't know if it is?)
| prop_state._heartbeat = false; | ||
| candidates.push_back({}); | ||
| navigation.next()++; | ||
| navigation.set_on_module(); |
There was a problem hiding this comment.
In principle at least, the navigator should figure this out on its own after you manipulate the current candidate. Can you explain what the issue is?
There was a problem hiding this comment.
I reset the candidate vector every actor call (candidates.clear()) so I have to set the status manually. Now you might wonder why I am resetting the vector and push the element (candidates.push_back(is)): In the detector and track setup I made in path_correction.inl where the track should make a full turn starting from a surface, the navigation would abort with the original navigation algorithm because it cannot find any candidate surface.
I expected that you would not like the actor but I thought it is better than making a new navigator. At least I didn't add it in the propagator/actors directory.
Of course, the most desirable way is enhancing the existing navigator algorithm so that we can use it without any redundant actor class.
There was a problem hiding this comment.
What I am afraid of is, that adding an unsafe method to a class interface inevitably leads to people using it also in other contexts, which can lead to weird side effects and bugs. So, yes, you are right unfortunately, I am not a big fan of this change :p
There was a problem hiding this comment.
In that sense, isn't set_statealso pretty dangerous?
There was a problem hiding this comment.
Its private and should hopefully only be callable by the navigator because its a friend class of the state
There was a problem hiding this comment.
But otherwise, you are right, of course, I would consider that dangerous, too
There was a problem hiding this comment.
There is one function that is even more dangerous, that I am also not happy about and that is set_volume. I just haven't figured out a better way yet. Probably we should use the state constructor to set the start volume of the propagation?
This PR depends on #291 #292 #293 #294 so it will be horrible to review this until other PRs get merged.
The actual changes include the followings:
covariance_engineandjacobian_enginewhich only worked on cartesian coordinateMeanwhile, I tried to handle acts-project/acts#1477 which claims that (1) the path correction is missing some terms and (2) the path correction should be added to
jacobian_transportrather than being multiplied.It seems that considering all terms make the covariance transport better while the addition of the path correction doesn't make it better. But It is much likely that I made some mistakes somewhere :( Anyway assuming I didn't make any stupid mistake, I currently followed the multiplication as implemented in Acts repository.