Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

1 change: 1 addition & 0 deletions MagneticField/Portable/BuildFile.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<use name="xtd"/>
14 changes: 14 additions & 0 deletions MagneticField/Portable/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

# MagneticField/Portable

This package will host simple and efficient implementations of the magnetic field map for use in well-defined scenarios on GPUs and other heterogeneous hardware.

## ParabolicMagneticField

The parabolic parametrisation of the magnetic field was optimised for extrapolation within R < 115 cm and |Z| < 280 cm, which is (almost) up to the surface of ECAL.
Beyond that it will diverge from the actual magnetic field map, especially at large eta, in ways that are hard to quantify.

This is based on a simple parabolic formula of Bz(z), neglecting the Br and Bphi components, and is thus vert fast. To account for the missing Br component, which contributes to transverse bending, the parametrization is tuned to reproduce as closely as possible the field integral computed over a straight path originated in (0,0,0) and across the whole tracker, rather than the value of Bz. More details can be found [here](https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideMagneticField#Alternative_parametrizations_of).



44 changes: 44 additions & 0 deletions MagneticField/Portable/interface/ParabolicMagneticField.h
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name PortableParametrizedTrackerField for the new package is misleading.
Based on the expected use case, isn't this going to be used at least up to ECAL ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, also the existing parametric magnetic field is not called "Tracker".

Copy link
Contributor

Choose a reason for hiding this comment

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

If a new package is the way to go, I would suggest to call it just MagneticField/Portable or something along that line.

Then we can add more options (the uniform MF, if anybody bothers with it instead of hard-coding the central value; and anything more precise we may come up with).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name PortableParametrizedTrackerField for the new package is misleading. Based on the expected use case, isn't this going to be used at least up to ECAL ?

This specific parametrization was optimized for extrapolation within R<115, |Z|<280 cm, which is indeed (almost) up to the surface of ECAL. But beyond that, it will diverge from the real map, especially at large eta, although I don't have numbers to quantify how much at hand.
Note that the original ParabolicParametrizedTrackerField, If called outside that range, it returns (0,0,0) and issues a LogDebug message. The isDefined method is used, in particular, to allow overlaying parametrizations of the inner region over the full CMS map, which is based on grid interpolation.

Indeed MagneticField/Portable leaves more flexibility for any future development one could possibly imagine. On the other hand, it will trick an unaware user to think it's the way to go for any application, i.e. to extrapolate to muon stations, where it will give nonsense. MagneticField/PortableParametrizations is possibly slightly better (somehow suggests that it provides only parametrizations of the real thing); nothing better comes to my mind at the moment.

PS: we already have a uniform MF option. cf. uniformMagneticField_cfi.py, that has notably been used to process cosmic data with B off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know that we have a uniform magnetic field in CMSSW, I meant for adding a "portable" one, like this parametric implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, it will trick an unaware user to think it's the way to go for any application, i.e. to extrapolate to muon stations, where it will give nonsense.

The name ParabolicParametrizedField has been used for the past 12 years. I don't think it "tricked" people then, and I don't think people were particularly wiser than today.

If there is a strong-enough reason to adopt a new name, the same change should be applied consistently to the alpaka and non-alpaka versions, but that is well beyond the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I pointed out a few times already, MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.h is part of the private implementation of the package, which was done exactly to prevent it being used directly.

We are now creating a new version that is exposed in the public interface, so we are giving direct access to people now, which has not been the case in the past 12 years.

In any case, it's not a matter of whose name is the longest. Having two files in different places with the same name is a not particularly wise choice, for reasons that should be obvious. If they are different, they should be called differently.

So, please avoid calling the new class with the same name of an already existing class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Having two files in different places with the same name is a not particularly wise choice, for reasons that should be obvious. If they are different, they should be called differently.

I agree that it would be better to use different names for completely different concepts.

Yes, I think it makes a lot of sense to use the same name for the same concept in different contexts.

See edm::EDProducer vs edm::one::EDProducer vs edm::stream::EDProducer vs edm::global::EDProducer vs ALPAKA_ACCELERATOR_NAMESPACE::stream::EDProducer vs ALPAKA_ACCELERATOR_NAMESPACE::global::EDProducer:

FWCore/Framework/interface/global/EDProducer.h
FWCore/Framework/interface/limited/EDProducer.h
FWCore/Framework/interface/one/EDProducer.h
FWCore/Framework/interface/stream/EDProducer.h
FWCore/Framework/interface/EDProducer.h
HeterogeneousCore/AlpakaCore/interface/alpaka/global/EDProducer.h
HeterogeneousCore/AlpakaCore/interface/alpaka/stream/EDProducer.h

Or edm::Event vs fwlite::Event vs ALPAKA_ACCELERATOR_NAMESPACE::device::Event:

DataFormats/FWLite/interface/Event.h
FWCore/Framework/interface/Event.h
HeterogeneousCore/AlpakaCore/interface/alpaka/Event.h

And so on.


Anyway, since you feel so strongly that you own the name ParabolicParametrizedField, let's pick a new subsystem, package and name for this.

Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#ifndef MagneticField_Portable_interface_ParabolicMagneticField_h
#define MagneticField_Portable_interface_ParabolicMagneticField_h

#include <xtd/stdlib/abs.h>

namespace portableParabolicMagneticField {

struct Parameters {
// These parameters are the best fit of 3.8T to the OAEParametrizedMagneticField parametrization.

static constexpr float c1 = 3.8114;
static constexpr float b0 = -3.94991e-06;
static constexpr float b1 = 7.53701e-06;
static constexpr float a = 2.43878e-11;
static constexpr float max_radius2 = 13225.f; // tracker radius
static constexpr float max_z = 280.f; // tracker z
};

template <typename Vec3>
constexpr float Kr(Vec3 const& vec) {
return Parameters::a * (vec[0] * vec[0] + vec[1] * vec[1]) + 1.f;
}

template <typename Vec3>
constexpr float B0Z(Vec3 const& vec) {
return Parameters::b0 * vec[2] * vec[2] + Parameters::b1 * vec[2] + Parameters::c1;
}

template <typename Vec3>
constexpr inline bool isValid(Vec3 const& vec) {
return ((vec[0] * vec[0] + vec[1] * vec[1]) < Parameters::max_radius2 && xtd::abs(vec[2]) < Parameters::max_z);
}

template <typename Vec3>
constexpr inline float parabolicMagneticFieldAtPoint(Vec3 const& vec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constexpr inline float parabolicMagneticFieldAtPoint(Vec3 const& vec) {
constexpr inline float magneticFieldAtPoint(Vec3 const& vec) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the name of the method with respect to MagneticField/ParametrizedEngine/interface/ParabolicParametrizedMagneticField.h ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't recall the reason for different name, restored the original one.

if (isValid(vec)) {
return B0Z(vec) * Kr(vec);
} else {
return 0;
}
}

} // namespace portableParabolicMagneticField
#endif
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
<bin file="alpaka/testParabolicParametrizedMagneticField.dev.cc">
<bin file="alpaka/testParabolicParametrizedTrackerField.dev.cc">
<use name="alpaka"/>
<use name="eigen"/>
<use name="DataFormats/GeometryVector"/>
<use name="FWCore/Utilities"/>
<use name="HeterogeneousCore/AlpakaInterface"/>
<use name="MagneticField/ParametrizedEngine" source_only="1"/>
Copy link
Contributor

@makortel makortel Nov 10, 2025

Choose a reason for hiding this comment

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

This line should be kept, but as

  <use name="MagneticField/Portable"/>

<use name="MagneticField/Portable"/>
<flags ALPAKA_BACKENDS="1"/>
</bin>
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,21 @@
#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"
#include "HeterogeneousCore/AlpakaInterface/interface/workdivision.h"
#include "MagneticField/ParametrizedEngine/interface/ParabolicParametrizedMagneticField.h"
#include "MagneticField/Portable/interface/ParabolicMagneticField.h"

using namespace edm;
using namespace std;
using namespace ALPAKA_ACCELERATOR_NAMESPACE;
using namespace magneticFieldParabolicPortable;
using namespace portableParabolicMagneticField;
using Vector3f = Eigen::Matrix<float, 3, 1>;

struct MagneticFieldKernel {
struct TrackerMagneticFieldKernel {
template <typename T>
ALPAKA_FN_ACC void operator()(Acc1D const& acc, T const* __restrict__ in, T* __restrict__ out, size_t size) const {
for (auto index : cms::alpakatools::uniform_elements(acc, size)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

...and this test should be moved as well.

out[index](0) = 0;
out[index](1) = 0;
out[index](2) = magneticFieldAtPoint(in[index]);
out[index](2) = parabolicMagneticFieldAtPoint(in[index]);
}
}
};
Expand Down Expand Up @@ -77,7 +77,8 @@ int main() {
field_host[i] = Vector3f::Zero();
}

float resolution = 0.2;
const float resolution = 0.2;

float maxdelta = 0.;
int fail = 0;

Expand All @@ -94,7 +95,7 @@ int main() {
alpaka::memset(queue, field_dev, 0.);

auto workDiv = cms::alpakatools::make_workdiv<Acc1D>(1, size);
alpaka::exec<Acc1D>(queue, workDiv, MagneticFieldKernel{}, points_dev.data(), field_dev.data(), size);
alpaka::exec<Acc1D>(queue, workDiv, TrackerMagneticFieldKernel{}, points_dev.data(), field_dev.data(), size);

// copy the results from the device to the host
alpaka::memcpy(queue, field_host, field_dev);
Expand All @@ -107,7 +108,7 @@ int main() {
const auto& point = points[i];
const auto& referenceB = referenceB_vec[i];
GlobalVector parametricB(field_host[i](0), field_host[i](1), field_host[i](2));
float delta = (referenceB - parametricB).mag();
const float delta = (referenceB - parametricB).mag();
if (delta > resolution) {
++fail;
if (delta > maxdelta)
Expand Down