[KEMField] Adapt data type of parameters nmax and nelliptic

[danielfrh]

Hi everyone!

Regarding the magnetic field solver MagfieldCoils within KEMField, the user can define values in the XML file:

Here n_max and n_ellptic are given. The user has to have the ability to adapt these values. These two values are of integer type. The code checks if the user-defined values are negative and responses with an error (or changes to a positive value).

=> In the the code the builder KMagfieldCoilsFieldSolverBuilder.cc takes both values as an unsigned int whereas the interface takes both values as integer (and the MagfieldCoils class takes int for both values too). This leads to misinterpretation of the user-given values and prevents from reading in the user-given values correctly. Instead always the default values from the constructor will be taken (nmax=500, n_elliptic=32).

Therefore I strongly suggest to change the data type in the corresponding builder class.

• A proposal can be found in my fork https://github.com/danielfrh/Kassiopeia.git in the branch danielfrh/MagfieldCoils.
• The error comes from the fact that a unsigned int has not the data width as a pure int which is needed for the class MagfieldCoils.
• It can be tested by adding couts to n_elliptic and n_max to the classes KMagfieldCoilsFieldSolverBuilder, KMagfieldCoilsFieldSolver and MagfieldCoils

Thanks,
Daniel

[2xB]

I think we should start by talking about this statement: "Instead always the default values from the constructor will be taken (nmax=500, n_elliptic=32).". Quoting from https://github.com/danielfrh/Kassiopeia/blob/8c4ed3a7c416a0be69033e1dc16c169a69c631a1/KEMField/Source/Bindings/FieldSolvers/Magnetic/include/KMagfieldCoilsFieldSolverBuilder.hh :

    if (aContainer->GetName() == "n_elliptic") {
        aContainer->CopyTo(fObject, &KEMField::KMagfieldCoilsFieldSolver::SetNElliptic);
        return true;
    }

There is no catch saying that integers should not be casted to unsigned integers or that in case of signed integers this should be skipped. Why would this code not be run then? Or does it run and throw an error message? Or is the casting from int to unsigned int not working? Every one of these options would confuse me.

[danielfrh]

Following happens:

In the binding class there is defined for the two parameters an unsigned, if the user defines now in the XML for example nmax=1234 and n_elliptic=12, these values don't reach the code, instead the default parameters 500/32 are taken. (Nb. all the different classes (except the builder :-) ) these two params are >>int<< not unsigned.)

[2xB]

Why don't the values "reach the code"? Until the line I quoted above, nothing in the code is specific to unsigned integers, so until then the values should be parsed. That if statement should then be executed and the code of that still calls a setter that unavoidably sets the value to some number that is not the default.

[2xB]

To be fully clear about why I care about this: When there is a type mismatch, I fully expect the configuration to not work and to get error messages, at least at runtime. Here it seems as if some code is just randomly skipped without an error, which would be frightening.

[danielfrh]

You are right the values reach the code, but the wrong values are taken. Because the conversion of unigned to int goes wrong, eg. n_max will be set to the max value of uint.

Since you cannot reproduce it( did you add a cout to the MagfieldCoils class and set the nmax and n_ellptic values to 123 eg in the XML input file?),
it could also be a compiler-OS constellation of myself which might not be too typical, but it should run? arm64 with clang
But an error without any message is frightening ;-)

[2xB]

I did not try to reproduce it yet, do you have a minimal working example XML configuration I can run with a debugger of my choice to observe this?

[danielfrh]

This is an example file for a calculation of fields.

You can run with TestCoils_SimFields.xml 1 TestCoils-Tilt2-ZH3 -r tilt=2 zh-config=3. Please note that you have to adapt the given paths in the file.

ZH3 is equal to the magfield coils solver in the XML file:

TestCoils_SimFields.xml
TestCoils_MagnetConfig.xml

[2xB]

Interesting, KContainer indeed does nothing when trying to run CopyTo against a target of wrong type. So this Pull Request is perfect, but I would like to try if there is any reason for not throwing a reasonable error in that case to catch such issues early in the future.

[2xB]

Here's a test running on whether we can just error in this case: 2xB#41 I am curious to see if Kassiopeia has more of such errors - or if the fact that this just did nothing and stayed silent was already used as some mechanics. Likely we will fall to Hyrums Law, but a bit of hope is left.

[2xB]

Also, I am wondering if we want to rename n_max and n_elliptic and throw an error if people use n_max and n_elliptic stating that those values never did anything and they should migrate to the new ones if they want to actually do something. Or if having this silent behavior change is okay. Maybe @richeldichel has an opinion there.

[2xB]

Here is a Pull Request on this: #161 We should be careful to first test it on as many configurations as possible before merging though to find as many broken bindings as possible already by hand.