Resolve "Implement a better format for the material parameters in CollimatorPhysics"

Closes #299 (closed)

added Enhancement label

added 1 commit

• c8d8bb54 - first possibility to better describe the materials

Compare with previous version

Just pushed a first version of a more structure material definition. The code works and I tested that the values are the same as before. Of course the CollimatorPhysics class needs to be adapted. I just wanted to keep the old stuff in to run the test that the values are correct.

I also have to change the variable name of the instantiations of the materials. Currently they're all called beryllium (because I did first Beryllium and then copied the files).

@snuverink_j what do you think? Do you have suggestions?

@kraus: looks good!

I would put the implementation of getMeanExcitationEnergy() in the base class to reflect the fact that it is one equation for all materials.

For all materials I could find the exact (??) value for the mean excitation energy PDG and NIST. In addition I'll add the getMeanExcitationEnergy() method to the Material class as default.

added 1 commit

• 9578b591 - use approximation of mean excitation energy as default in Material class and...

Compare with previous version

added 1 commit

• a2b50970 - fix reference for case when all particles are in degrader material; refactor...

Compare with previous version

added 3 commits

• a2b50970...dfd6fa26 - 2 commits from branch master
• e3ae181b - Merge branch 'master' into...

Compare with previous version

added 1 commit

• d0547eeb - clean up

Compare with previous version

unmarked as a Work In Progress

resolved all discussions

@gsell & @frey_m could you have a look at the changes and give me feedback (or approve)? I'm ready to push.

@kraus I approve, however, I didn't check the hard-coded atomic values for correctness.

approved this merge request

approved this merge request

@kraus I just realized that currently all materials implement the getter-functions. Wouldn't it be better to implement those in the material base class. The values (e.g. atomic number) could be given to the material base class constructor. Thus, we could make the code more compact.

@frey_m I did check that values are still the same. Have a look at the first commit in this merge request. I

To your second comment: this was my first idea as well. But I decided against it. I don't like methods with many arguments. Currently there are 9 values and in the future possibly even more. @snuverink_j what's your opinion on this?

@kraus I agree with @frey_m. Only the constructor needs many arguments.

One minor point: Some line are pretty long. Wrapping them makes the code better readable.

1. It depends if you'd like to have the same member implementation in all the material classes it makes sense to put the members (and access implementation) in the base class. If you'd like to keep the implementation for each derived class flexible, then the code as it is seems good to me.

2. You don't need to have a constructor with many arguments per se. You can also make the members protected and set the values in each derived class in the default constructor directly. Nevermind, that proposal is error-prone (not all variables might be set).

I don't have a strong opinion. The code is pretty clean and the additional typing seems manageable.