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

Implementing direct navigator #948

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

beomki-yeo
Copy link
Collaborator

@beomki-yeo beomki-yeo commented Mar 7, 2025

This PR implements direct navigator.
The direct navigator runs on the input surface sequence which include all types of surfaces (passive, sensitive and portals)
The input sequence can be collected from previous run of propagator using barcode_sequencer actor
It is also designed to work with both forward and backward directions.

The PR also fixes the bug that path is not written in the cylinder intersections when path < overstep tolerance, which occasionally killed the propagation with direct navigator and small overstep tolerance.

Also This PR sets the volume in the propagator constructor with bound parameter inputs.

@beomki-yeo beomki-yeo marked this pull request as draft March 7, 2025 08:22
@beomki-yeo beomki-yeo marked this pull request as ready for review March 11, 2025 17:33
@beomki-yeo beomki-yeo requested a review from niermann999 March 11, 2025 17:37
@beomki-yeo beomki-yeo added enhancement New feature or request priority: high high priority labels Mar 11, 2025
@beomki-yeo beomki-yeo force-pushed the direct-navigator branch 4 times, most recently from 263822f to b84990a Compare March 11, 2025 22:53
@beomki-yeo beomki-yeo added the bug Something isn't working label Mar 11, 2025
@beomki-yeo beomki-yeo force-pushed the direct-navigator branch 12 times, most recently from 2df589c to 4263639 Compare March 12, 2025 08:26
vecmem::data::vector_buffer<detray::geometry::barcode> seqs_buffer{
100u, host_mr, vecmem::data::buffer_type::resizable};
vecmem::copy m_copy;
m_copy.setup(seqs_buffer)->wait();
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is quite complicated for host code. A device vector can simply be built from the view of a vecmem::vector, or not?

Copy link
Collaborator Author

@beomki-yeo beomki-yeo Mar 12, 2025

Choose a reason for hiding this comment

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

Resizable buffer needs this copy.setup to build a device vector

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, but I don't understand why you need a buffer in the first place? Does the following not work?

vecmem::vector<geometry::barcode> seq_vector{host_mr};
auto seq_view = detray::get_data(seq_vector);

Copy link
Collaborator Author

@beomki-yeo beomki-yeo Mar 12, 2025

Choose a reason for hiding this comment

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

That will be easier in the host test.
However, during the GPU KF, vecmem::buffer will be much easier to use. So the code above is the proof of concept for the KF implementation

Comment on lines 348 to 351
sf.is_portal() ? darray<scalar_type, 2>{cfg.min_mask_tolerance,
cfg.max_mask_tolerance}
: darray<scalar_type, 2>{cfg.min_mask_tolerance,
cfg.max_mask_tolerance},
static_cast<scalar_type>(cfg.mask_tolerance_scalor),
static_cast<scalar_type>(cfg.overstep_tolerance));
Copy link
Contributor

Choose a reason for hiding this comment

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

Tha mask tolerance and overstepping tolerance are not needed for direct navigation, I believe

Suggested change
sf.is_portal() ? darray<scalar_type, 2>{cfg.min_mask_tolerance,
cfg.max_mask_tolerance}
: darray<scalar_type, 2>{cfg.min_mask_tolerance,
cfg.max_mask_tolerance},
static_cast<scalar_type>(cfg.mask_tolerance_scalor),
static_cast<scalar_type>(cfg.overstep_tolerance));
darray<scalar_type, 2>{cfg.min_mask_tolerance,
cfg.min_mask_tolerance},
0.f,
-detail::invalid_value<scalar_type>());

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without mask tolerance, the surfaces can get missed during the backward propagation.

The overstep tolerance could be any values technically but I worry the situation where the navigation is trapped in an infinite loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

The direct navigator should not miss a surface when the intersection is outside or it would run precisely into the same problems the current navigator does. Or am I missing something?

The overstep tolerance could be any values technically but I worry the situation where the navigation is trapped in an infinite loop.

I am sorry, I cannot follow on how it could get stuck this way. Can you explain what you mean, please?

Copy link
Collaborator Author

@beomki-yeo beomki-yeo Mar 12, 2025

Choose a reason for hiding this comment

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

Anyway you asked..
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, OK, that is interesting! Can you show me how to reproduce this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And in our geometries during tracking, is there a configuration you found that I can try?

Just make a telescope geometry if you want to try. Do you need a exact number and codes? No way that I am going to write it for you. You just don't like this or what??

Copy link
Contributor

Choose a reason for hiding this comment

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

I was asking, because I would like to debug this case and was wondering if you had already encountered this and could give me a starting point... that is all, I am sorry

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think you would ever try it on your own. you just want to prove that I have not encountered this actually so shut down my argument, don't you?

As I said, set it to -MAX with the configuration object if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this problem makes sense to me and I had not thought of it before. I am genuinely sorry if you feel that way

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤡

@beomki-yeo beomki-yeo requested a review from niermann999 March 12, 2025 21:32
@beomki-yeo beomki-yeo force-pushed the direct-navigator branch 2 times, most recently from 166c9a6 to 7845fad Compare March 13, 2025 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request priority: high high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants