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

Put field into stepper state #294

Merged
merged 14 commits into from
Oct 12, 2022

Conversation

beomki-yeo
Copy link
Collaborator

@beomki-yeo beomki-yeo commented Aug 30, 2022

Depends on #293.
Actually it was a painful job because the stepper is used in the telescope geometry. In my opinion, we should use ray and helix instead...

@niermann999 I made create_telescope_detector take ray/helix instead of track_parameters

@beomki-yeo beomki-yeo requested a review from niermann999 August 30, 2022 23:06
@@ -68,6 +73,8 @@ class rk_stepper final : public base_stepper<track_t, constraint_t, policy_t> {
array_t<scalar, 4> k_qop;
} _step_data;

const magnetic_field_t _magnetic_field;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a good idea? A stepper state exists for every track as part of the propagator state, which would mean we end up with thousands of b-field copies if we don't pass this by pointer. Also, the b-field [pointer] in the stepper instead of the e.g. the propagator state means, we get a b-field data member in the line stepper, which does not seem right in terms of inheritance. For the navigator we have the tracking geometry as a pointer already. I know its bad design to have to guarantee the lifetime of the data outside the class that uses it, but I don't see a good way to do it differently?

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 agree. I changed it to a pointer

@niermann999
Copy link
Contributor

Depends on #293. Actually it was a painful job because the stepper is used in the telescope geometry. In my opinion, we should use ray and helix instead...

Ok, I was just lazy to calculate the surface placement myself when the stepper essentially does the same thing already :D

@beomki-yeo
Copy link
Collaborator Author

Yeah Using ray or helix would make the interface easier.
BTW, I much like that it supports a bent track for surface placement. This can build the tracking geometry of g-2 at J-PARC

@beomki-yeo
Copy link
Collaborator Author

It's also ready

@beomki-yeo
Copy link
Collaborator Author

@niermann999 Do you have more comments for this PR?

@niermann999
Copy link
Contributor

@niermann999 Do you have more comments for this PR?

Oh, sorry, I forgot about this one. Please go ahead

@niermann999
Copy link
Contributor

Can we merge my PRs first, though?

@beomki-yeo
Copy link
Collaborator Author

It's also rebased

@niermann999
Copy link
Contributor

This probably only works precisely for homogeneous b fields, come to think of it, but I don't think that's a problem, so let's get this in

@niermann999 niermann999 merged commit 9fb93a3 into acts-project:main Oct 12, 2022
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