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

Refactor the Parameter interface for Kalman fitting test #774

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Nov 15, 2024

In the current main branch, all tests derived from KalmanFittingTests share the same input parameters.
This made it impossible to give the input parameters specific to detector types (e.g. Telescope, toy geometry and wire chamber).

In this PR, KalmanFittingTests does not take input parameters anymore. Instead, KalmanFittingTelescopeTests, KalmanFittingToyDetectorTests, and KalmanFittingWireChamberTests have their own input parameters.

KalmanFittingTelescopeTests also sets the planes (20 mm, 40 mm, ..., 180 mm) instead of (0 mm, 20 mm, ..., 160 mm)

@niermann999
Copy link
Contributor

KalmanFittingTelescopeTests also sets the planes (20 mm, 40 mm, ..., 180 mm) instead of (0 mm, 20 mm, ..., 160 mm)

Be careful to either move the track generator origin into the detector space or set cfg.envelope(21mm) such that the portals encompass the position (0,0,0). Otherwise the navigation might start outside of the detector...

detray::tel_det_config<> tel_cfg{rectangle};
tel_cfg.positions(plane_positions);
tel_cfg.module_material(mat);
tel_cfg.mat_thickness(thickness);
tel_cfg.pilot_track(traj);
tel_cfg.envelope(offset * 2.f);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is good enough

@beomki-yeo
Copy link
Contributor Author

Review anyone?

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Okay... I don't have particularly strong opinions about this testing code. 🤔

The one thing I noticed: Should the SYCL test not be updated as well?

https://github.com/acts-project/traccc/blob/main/tests/sycl/test_kalman_fitter_telescope.sycl

@beomki-yeo
Copy link
Contributor Author

It's been a while since SYCL KF test was disabled. (See #655)

Maybe there are more things to fix other than the change in this PR. So I suggest to do it with a dedicated PR

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Fine, let's do SYCL separately. I believe we'll need oneAPI 2025.0.0 Docker images as a first step... (And then we'll need to update a bunch of code to be compatible with the new version...)

@beomki-yeo beomki-yeo merged commit 3350e50 into acts-project:main Nov 24, 2024
26 checks passed
flg pushed a commit to flg/traccc that referenced this pull request Nov 26, 2024
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.

3 participants