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

Replace constant magnetic field with covfie fields #311

Merged

Conversation

stephenswat
Copy link
Member

This change migrates all existing usage of magnetic fields -- all constant -- to use the covfie library. From here, integrating analytical solenoidal fields or data-based fields should be relatively trivial.

@stephenswat stephenswat added the refactor refactoring the current codes label Oct 11, 2022
@stephenswat stephenswat changed the title Replace constant magnetic field with covfie field Replace constant magnetic field with covfie fields Oct 11, 2022
Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

This looks really nice and integrates into the detector view much better than I thought!

core/include/detray/core/detector.hpp Outdated Show resolved Hide resolved
core/include/detray/core/detector.hpp Show resolved Hide resolved
core/include/detray/propagator/rk_stepper.ipp Outdated Show resolved Hide resolved
// Use rectangular surfaces
constexpr bool rectangular = false;

using b_field_t = decltype(create_telescope_detector<rectangular>(std::declval<vecmem::host_memory_resource&>(), std::declval<std::vector<scalar>&>()))::bfield_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, same goes for the telescope of course, but I have to fix that detector anyways, so I will look into this

@@ -273,8 +276,12 @@ auto create_telescope_detector(
scalar m_thickness;
};

typename detector_t::bfield_type bfield(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, nice, then I just have to make it configurable somehow

@niermann999
Copy link
Contributor

Do you mind if we merge #294 first though?

@stephenswat
Copy link
Member Author

Of course!

@stephenswat stephenswat force-pushed the feat/covfie_constant_field branch from f53d1a2 to 23e2466 Compare October 12, 2022 12:20
@stephenswat
Copy link
Member Author

This PR has been rebased.

@stephenswat stephenswat force-pushed the feat/covfie_constant_field branch 3 times, most recently from 80027fa to 1c36017 Compare October 12, 2022 13:09
@stephenswat
Copy link
Member Author

Formatting has been adjusted, all tests pass.

@stephenswat stephenswat force-pushed the feat/covfie_constant_field branch 2 times, most recently from 9229a6a to e46a5be Compare October 12, 2022 14:31
This change migrates all existing usage of magnetic fields -- all
constant -- to use the covfie library. From here, integrating analytical
solenoidal fields or data-based fields should be (relatively) trivial.
@stephenswat stephenswat force-pushed the feat/covfie_constant_field branch from e46a5be to 0b5d29b Compare October 12, 2022 14:41
@@ -37,7 +37,6 @@ class rk_stepper final
using point3 = typename transform3_type::point3;
using vector2 = typename transform3_type::point2;
using vector3 = typename transform3_type::vector3;
using context_type = typename magnetic_field_t::context_type;
Copy link
Contributor

Choose a reason for hiding this comment

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

I will have a look shortly how to handle the context objects, so let's see

const auto d = create_toy_geometry(host_mr, n_brl_layers, n_edc_layers);

// Construct the constant magnetic field.
using b_field_t = decltype(
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we just need to find a way to make the backend configurable from here, but let's do that when we switch to the solenoid

@niermann999
Copy link
Contributor

Let's get this in asap

@niermann999 niermann999 merged commit 0ca494d 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
refactor refactoring the current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants