Parametrized magnetic field in alpaka

[lecriste]

PR description:

Preliminary PR to access the magnetic field approximation in alpaka. No changes are expected in the output.

Triggered by last bullet in https://docs.google.com/presentation/d/1WVF6F0CHImtcGzj-Pnyvo6nEBkTJokDsJx1JyxwLtJo/edit#slide=id.g2c0e7808fad_23_360

[cmsbuild]

cms-bot internal usage

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39400

• This PR adds an extra 20KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

• code-format: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39400/code-format.patch e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39400/code-format.patch | patch -p1 You can also run scram build code-format to apply code format directly

[jfernan2]

assign heterogeneous

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39401

• This PR adds an extra 20KB to repository

[cmsbuild]

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

A new Pull Request was created by @lecriste for master.

It involves the following packages:

• MagneticField/ParametrizedEngine (reconstruction)

@jfernan2, @fwyzard, @cmsbuild, @makortel, @mandrenguyen can you please review it and eventually sign? Thanks. @namapane this is something you requested to watch as well. @sextonkennedy, @antoniovilela, @rappoccio you are the release manager for this.

cms-bot commands are listed here

[namapane]

I'd like to add a warning here. This class provides a fast, rough approximation that is intended to be used within the tracker region only. The nominal region of validity is defined here:

https://github.com/cms-sw/cmssw/blob/master/MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.cc#L44

It is generally a bad idea to use this approximation outside this region.

Also, please note that the header is placed in src on purpose, because this class is not intended to be used directly by other packages - this is just part of the implementation of the plugin that the package implements. The MF is a service, which is served depending on the conditions (magnet current). Hardcoding a piece of implementation elsewhere will break this dependence, and one may at some point have a very hard time to figure out that some piece of code is using the wrong field for, e.g., runs at B=0 or if we ever have to take data at reduced magnet currents.

I would really advise to use the MF in the way it is designed and used in the rest of CMSSW, and not to make some pieces available in the public interface of the package.

[makortel]

@namapane

served depending on the conditions (magnet current). Hardcoding a piece of implementation elsewhere will break this dependence, and one may at some point have a very hard time to figure out that some piece of code is using the wrong field for, e.g., runs at B=0 or if we ever have to take data at reduced magnet currents.

Just to make sure I've understood correctly, are all of the conditions effectively encoded in the four parameters (c1, b0, b2, a) of the ParabolicParametrizedMagneticField?

Would this concern be satisfied if the parameters would have to be obtained from the magnetic field object in the EventSetup?

I would really advise to use the MF in the way it is designed and used in the rest of CMSSW, and not to make some pieces available in the public interface of the package.

The challenge is that the existing magnetic field infrastructure is not usable in GPU, which is what I believe @lecriste et al would need to do (in one way or another).

[lecriste]

The challenge is that the existing magnetic field infrastructure is not usable in GPU, which is what I believe @lecriste et al would need to do (in one way or another).

Right, and we verified that for the use-case of the e/gamma seeding, the parabolic approximation is suitable to be adopted at the ECAL boundary.

[cmsbuild]

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39402

• This PR adds an extra 20KB to repository

• Found files with invalid states:

  • MagneticField/ParametrizedEngine/plugins/alpaka/ParabolicParametrizedMagneticField.h:
    • Added: 877e601a9399fb6400512b3a3ade17ff6666fd53
    • Modified: b138ff1cce531dfb9ab55f9383b29fdbb0b0c4e7
    • Deleted: 0146963bd4ffdc6be05d7af10162562ab62d5f95

Code check has found code style and quality issues which could be resolved by applying following patch(s)

• code-format: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39402/code-format.patch e.g. curl -k https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39402/code-format.patch | patch -p1 You can also run scram build code-format to apply code format directly

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39403

• This PR adds an extra 16KB to repository

• Found files with invalid states:

  • MagneticField/ParametrizedEngine/plugins/alpaka/ParabolicParametrizedMagneticField.h:
    • Added: 877e601a9399fb6400512b3a3ade17ff6666fd53
    • Modified: b138ff1cce531dfb9ab55f9383b29fdbb0b0c4e7
    • Deleted: 90c61284ea0cd4ce9762dafb8408405d144b4555

[cmsbuild]

Pull request #44360 was updated. @fwyzard, @makortel, @mandrenguyen, @jfernan2, @cmsbuild can you please check and sign again.

[namapane]

Just to make sure I've understood correctly, are all of the conditions effectively encoded in the four parameters (c1, b0, b2, a) of the ParabolicParametrizedMagneticField?

No, these are the parameters for 3.8T. The idea is that the MF (both full version and parametrizations for the tracker region) are provided by a ESProducer based on standard sequences (cf StandardSequences/python/MagneticField_cff.py. These ESProducers configure the field for the operating conditions (ie. depending on RunInfo). For parametrizations, the ESProducer is AutoParametrizedMagneticFieldProducer. First of all, it creates a zero-field uniform map if the current is zero (regardless of the type of parametrization that is required). Otherwise, for ParabolicParametrizedMagneticField, it scales parameters to the closest nominal magnet working point and passes them to the ParabolicParametrizedMagneticField ctor here. Actually, the parameters are present in the default ctor of ParabolicParametrizedMagneticField for historical reasons only, as this was the initial implementation. The default ctor would better be removed as parameters are set properly by AutoParametrizedMagneticFieldProducer.

I am in general uncomfortable with adding shortcuts which override some of the intended functionality (in this case, the dependence from RunInfo).

The challenge is that the existing magnetic field infrastructure is not usable in GPU, which is what I believe @lecriste et al would need to do (in one way or another).

That is probably the case for a good share of ESProducers in CMSSW... at some point I think that if it there is no other solution, it may be better to make a class that is completely independent, rather than hacking a piece of a ESProducer which will silently behave differently than the real thing. At least it will be clear what it does...

[fwyzard]

-heterogeneous

[fwyzard]

@lecriste can we discuss what you are trying to achieve and the implementation details at the GPU meeting next Monday ?

[lecriste]

@fwyzard sure, I will join the meeting. I opened this small PR after the Patatrack Hackathon firstly to reach all the involved parties. As mentioned in the PR description, we want to access the magnetic field approximation in alpaka at the ECAL boundary, where its value serves well the purpose of electrons trajectory propagation.

[cmsbuild]

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44360/39409

• This PR adds an extra 20KB to repository

• Found files with invalid states:

  • MagneticField/ParametrizedEngine/plugins/alpaka/ParabolicParametrizedMagneticField.h:
    • Added: 877e601a9399fb6400512b3a3ade17ff6666fd53
    • Modified: b138ff1cce531dfb9ab55f9383b29fdbb0b0c4e7
    • Deleted: 90c61284ea0cd4ce9762dafb8408405d144b4555

[cmsbuild]

Pull request #44360 was updated. @makortel, @mandrenguyen, @jfernan2, @fwyzard, @cmsbuild can you please check and sign again.

[makortel]

The challenge is that the existing magnetic field infrastructure is not usable in GPU, which is what I believe @lecriste et al would need to do (in one way or another).

That is probably the case for a good share of ESProducers in CMSSW...

True in the sense that pretty much only those ES data products that are explicitly GPU compatible can be used on GPUs. I believe, however, in most cases it is more or less straightforward to develop an ESProducer that converts an existing ES data product to another data product that is usable in GPU.

What makes magnetic field infrastructure complicated is its use of dynamic polymorphism and abstract interface (i.e. clients call only virtual functions of the MagneticField base class). While this is great from software design standpoint, it is generally difficult pattern for GPUs (or at least has been).

Just thinking out loud, I'm wondering if a feasible compromise would be to craft an ESProducer that converts an existing MagneticField into a simple-enough parametric object (that can be used on GPU), only if the concrete MagneticField type is ParabolicParametrizedMagneticField. Working around the abstraction of MagneticField is certainly a code smell, but maybe a better solution could be figured out in the future?

[namapane]

@makortel, the reason why the MF infrastructure is so complicated is that it has to serve many different use cases with different requirements, while respecting the dependence from RunInfo and geometry within the CMSSW framework design. Parametrizations are just specific use cases which cover only a subregion of the inner solenoid. The main use case is simulation and tracking within the entire CMS, including the yoke, where no parametrization is possible; this requires a much more complicated infrastructure (incl geometry for example). All this should be completely transparent to the user that gets the MF as a service with a standard interface. I'm not fluent in GPU restrictions but possibly it may make sense to create a "standalone" ESProducer that creates a GPU-compatible MF provider that is configured depending on RunInfo, with parameters set explicitly - I would not bother picking them from ParabolicParametrizedMagneticField given that the default ctor includes just those for 3.8T, and in any case these parameters are not going to change in any foreseeable future. My main concern is that such an "hardcoded" version may get used in other contexts where it is not appropriate, instead of the real thing - eg users looking around for an example and picking this as a model, for example to extrapolate to HCAL or for muon reconstruction, where it makes no sense. The current implementation in this PR makes no validity check so it will not warn even if it is called for a point outside the solenoid.

[makortel]

@namapane Do I understand correctly that your preference would be to replicate this logic (plus the surrounding code it needs) https://github.com/cms-sw/cmssw/blob/2e5dc6dfc7a5b5c7fc69b46f79859ee02b97e3b3/MagneticField/ParametrizedEngine/plugins/AutoParametrizedMagneticFieldProducer.cc#L74-L85 for obtaining the parameters into a separate ESProducer that would produce another ES data product, that would be explicitly "ParabolicParametrized" and "GPU friendly"?

(ok, written that, rather than replicating the logic, it should be possible, and possibly feasible, to abstract the commonalities between AutoParametrizedMagneticFieldProducer and the new ESProducer, and between ParabolicParametrizedMagneticField and the new ES product)

I'm not fluent in GPU restrictions

In the leading order classes that are trivially copyable and have constexpr methods are straightforward to use on GPU.

[namapane]

@namapane Do I understand correctly that your preference would be to replicate this logic (plus the surrounding code it needs) [...] for obtaining the parameters into a separate ESProducer that would produce another ES data product, that would be explicitly "ParabolicParametrized" and "GPU friendly"?

Yes.

(ok, written that, rather than replicating the logic, it should be possible, and possibly feasible, to abstract the commonalities between AutoParametrizedMagneticFieldProducer and the new ESProducer, and between ParabolicParametrizedMagneticField and the new ES product)

Depends on how this is achieved code-wise... As you noted the current infrastructure is already rather complex and if we have to make one (and only one, as a special case) concrete MF provider to conform to one more infrastructure, it may make things oddly complicated... At the end, this parametrization is 100% trivial, it's just a polynomial for a single component of the field. I agre it would definitely make sense to spend some effort to abstract commonalities if at some point we need to port more accurate parametrizations (eg the default one, which models Br in addition to Bz, and in a wider region within the solenoid).

[lecriste]

Hi @namapane,

I'm not fluent in GPU restrictions but possibly it may make sense to create a "standalone" ESProducer that creates a GPU-compatible MF provider that is configured depending on RunInfo,

With "configured" do you mean the parameters scaling according to the magnet current value? I don't see any other potential configuration apart from the if (cnc == 0) { version = "Uniform"; ... }.

My main concern is that such an "hardcoded" version may get used in other contexts where it is not appropriate, instead of the real thing - eg users looking around for an example and picking this as a model, for example to extrapolate to HCAL or for muon reconstruction, where it makes no sense. The current implementation in this PR makes no validity check so it will not warn even if it is called for a point outside the solenoid.

We can add a validity check up to the ECAL boundary (use-case of this PR) and return 0 outside. Sounds good?

[namapane]

Hi @lecriste ,

With "configured" do you mean the parameters scaling according to the magnet current value? I don't see any other potential configuration apart from the if (cnc == 0) { version = "Uniform"; ... }.

Yes; as I mentioned, this is a super-trivial parametrization. Note: scaling is not to the actual value recorded in RunInfo, but to the closest nominal working point, as implemented there. cnc=0 should be configured so that it returns B=0 everywhere.

We can add a validity check up to the ECAL boundary (use-case of this PR) and return 0 outside. Sounds good?

Yes, that would at least avoid completely crazy results. The standard implementation of inTesla() does the same, plus it also issues a LogDebug (https://github.com/cms-sw/cmssw/blob/master/MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.cc#L25-L33). (The MagneticField interface specifies also an inTeslaUnchecked() member, which is intended for users who want to save the additional CPU for the check, aka "I know what I'm doing")

[lecriste]

Alright. I think we can add the ECAL boundary validity and leave the parameters scaling for a future refinement when the use-case (e/gamma seeding) served by this PR is mature enough. Shall we get these four parameters values from this namespace rather than hardcoding them again?

[namapane]

Shall we get these four parameters values from this namespace rather than hardcoding them again?

Fine for me providing that this does not require to move this header to the public's package interface. This is and should remain a detail of the implementation of the package and I'd like to avoid that other users start to pick it up freely. At the end, it's just 4 numbers; also, they have been optimized for extrapolation within the tracker region. Conceptually, there is no special reason to force them to be the identical for a different use case. While there may be no practical need to re-optimize the parameter for extrapolation to ECAL, it is certainly possible; so hardcoding the parameters is not really such a bad thing IMO.

[makortel]

Shall we get these four parameters values from this namespace rather than hardcoding them again?

Fine for me providing that this does not require to move this header to the public's package interface.

Do you refer to MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.h? Keeping that header private (i.e. in src) is fine as such, but the definition of the parameters added in this PR https://github.com/cms-sw/cmssw/blob/bcda288b577afe369922c4534bcb7034f6489862/MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.h#L19-L25 must then be moved to another header in the public interface in order to be usable in other packages.

[namapane]

Shall we get these four parameters values from this namespace rather than hardcoding them again?

Fine for me providing that this does not require to move this header to the public's package interface.

Do you refer to MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.h? Keeping that header private (i.e. in src) is fine as such,

Yes.

but the definition of the parameters added in this PR

https://github.com/cms-sw/cmssw/blob/bcda288b577afe369922c4534bcb7034f6489862/MagneticField/ParametrizedEngine/src/ParabolicParametrizedMagneticField.h#L19-L25

must then be moved to another header in the public interface in order to be usable in other packages.

Exactly. My point is that I would rather hardcode parameters in the alpaka header than exposing a bunch of numbers in the package interface, since these are not universal parameters, they are good for a very specific use case and, at least ideally, they should be retuned for the ECAL extrapolation used here (which we may not bother doing, but conceptually, there is no reason to force them to be the same).