Variable CSA response curve

[stephanlachnit]

Closes #17

@simonspa so this would be the my initial idea for the implementation. It's not polished by any means, just a concept.

I thought figured offloading everything to a different files makes changes or additional implementations in the future easier. Especially with the impulse response being part of a class, any type of response can be implemented. In theory, one could even move the entire pulse amplification loop to the class, to implement features like saturation.

[stephanlachnit]

@simonspa this is now how I would implement it. I tested the simple type and I works as expected. I still need to check some corner cases for the tf1 type, but in principle everything should be there.

I would add a CI test and update the documentation once you're happy with the implementation.

[stephanlachnit]

Btw the github CI needs to be updated for the latest commits:

|16:19:25.553|   (FATAL) Error in the configuration:
                         Key 'workers' in global section does not exist
                         The configuration needs to be updated. Cannot continue.

[stephanlachnit]

Alright I tested if the function is invalid and if the amount of parameters given isn't the same as the function takes, both throw as expected. In the first case, I found no way to disable the output of the root error message, but that isn't necessarily bad as it helps to understand what's wrong in the formula. It isn't pretty though.

One open question is the default value. Right now I just use some formula with random values that roughly imitate what the MuPix10 does. However, I think it's rather ugly. How does simply returning 0 sound to you?

[schuetzepaul]

Btw the github CI needs to be updated for the latest commits:

|16:19:25.553|   (FATAL) Error in the configuration:
                         Key 'workers' in global section does not exist
                         The configuration needs to be updated. Cannot continue.

That's an interesting behaviour... I will have a look at this, thanks!

[simonspa]

Hej @stephanlachnit

thanks for the ping - this approach to me looks very elegant and clean, you're using the same code for all models. I have two (very nit-picking) ideas:

• How about making the TF1* a std::unique_ptr<TF1>? Just for the sake of not having raw pointers flying around. Not that it would change something.
• How about naming the new parameters response_function and response_parameters? Not sure about this, but it makes the relation a bit more clear? @schuetzepaul opinion?
• I'm thinking about the model = "tf1", maybe we could come up with a more "speaking" parameter name? For the internal type it's fine though...

Apart from that: 🚀

• If you'd add some unit tests and the documentation, I'd say this is ready to go.
• I would simply not set a default as response function. This basically forces the user to provide it when selecting model = TF1.
• Concerning the Mupix10 response - how about adding this as one of the examples to the README's last section?
• For now, don't worry about the CI, there seems to be a more fundamental issue somewhere... :)

Thanks a lot, nice piece of code! :)

Simon

[stephanlachnit]

How about making the TF1* a std::unique_ptr? Just for the sake of not having raw pointers flying around. Not that it would change something.

Oh yeah good idea.

How about naming the new parameters response_function and response_parameters? Not sure about this, but it makes the relation a bit more clear?

Agreed.

I'm thinking about the model = "tf1", maybe we could come up with a more "speaking" parameter name? For the internal type it's fine though...

I don't like it either, but I don't a fitting name for it. variable maybe? At least when combined with "variable model" it makes more sense then for example function. Or custom, that would also work.

If you'd add some unit tests and the documentation, I'd say this is ready to go.

Alright then I will start working on that.

Concerning the Mupix10 response - how about adding this as one of the examples to the README's last section?

Good idea.

[simonspa]

I don't like it either, but I don't a fitting name for it. variable maybe? At least when combined with "variable model" it makes more sense then for example function. Or custom, that would also work.

custom sounds great to me!

[stephanlachnit]

Ok I think I'm almost done. Two things left:

• Feedback on the documentation. I'm not the best with words.
• I don't quite understand the comment at the bottom of the unit tests. How should I pick them? Are the unit tests just supposed to pass with no throwing error or can we check internal values?

[simonspa]

Minor detail: Feel free to pile up several commits, that makes them more atomic and in the end eases finding a problem later on e.g. using git bisect. Otherwise it just points to the large blob of code that adds a com0pletely new feature.

[simonspa]

Concerning the test cases:

these current are rather system tests than unit tests, meaning we have no introspection into the code but simply parse the output of a single-event run with the relevant modules active. This approach is described here in the manual:
https://project-allpix-squared.web.cern.ch/project-allpix-squared/usermanual/allpix-manualch10.html#x11-22700010.6

The PASS condition is picked up automatically by CTest and is compared as regex against the stdout of the simulation defined by the test config. There is an additional PASSOSX because unfortunately STL does not guarantee implementations of random distributions, but only of the underlying generators. This means we get divergent results on different platforms. At some point in the future we might want to ship our own distribution implementations to solve this... :)

[stephanlachnit]

Ok thanks for the explanation for the unit tests. I now use

#PASS [R:CSADigitizer:mydetector] Pixel (2,0): time 7clk, signal 515clk
#PASSOSX [R:CSADigitizer:mydetector] Pixel (2,0): time 7clk, signal 515clk

since this will check if the function is actually applied. I could also use

#PASS [C:CSADigitizer:mydetector] Response function successfully initialized with 3 parameters
#PASSOSX [C:CSADigitizer:mydetector] Response function successfully initialized with 3 parameters

which describes a bit more what is actually tested, but is a bit weaker.

[stephanlachnit]

Also regarding the documentation, in https://github.com/allpix-squared/allpix-squared/blob/master/doc/usermanual/chapters/testing.tex a lot of tests are missing so I didn't add the new one, should I add it?

[simonspa]

Ok thanks for the explanation for the unit tests. I now use

#PASS [R:CSADigitizer:mydetector] Pixel (2,0): time 7clk, signal 515clk
#PASSOSX [R:CSADigitizer:mydetector] Pixel (2,0): time 7clk, signal 515clk

since this will check if the function is actually applied. I could also use

#PASS [C:CSADigitizer:mydetector] Response function successfully initialized with 3 parameters
#PASSOSX [C:CSADigitizer:mydetector] Response function successfully initialized with 3 parameters

which describes a bit more what is actually tested, but is a bit weaker.

You could also just add two tests :)

[simonspa]

Also regarding the documentation, in https://github.com/allpix-squared/allpix-squared/blob/master/doc/usermanual/chapters/testing.tex a lot of tests are missing so I didn't add the new one, should I add it?

I still hope to get that autogenerated from CMake at some point: https://gitlab.cern.ch/allpix-squared/allpix-squared/-/issues/207 - so from my point of view: don't bother. :)

[stephanlachnit]

You could also just add two tests :)

Yup, just did. Note though: maybe add a trailing zero in the second number of the tests like with the first number (e.g. test_06-09), right now ctest doesn't execute them in the natural order.

I think I fixed everything you pointed out. I will probably come back to the examples and exchange some values for the MuPix10 when I have better ones.

[stephanlachnit]

Ah found typos. Fixed now (hopefully).

[simonspa]

Awesome, thanks a lot, @stephanlachnit for all your work and this very neat feature!

Final CI run on Gitlab here: https://gitlab.cern.ch/allpix-squared/allpix-squared/-/merge_requests/455
and then I'm merging this.

[simonspa]

You could also just add two tests :)

Yup, just did. Note though: maybe add a trailing zero in the second number of the tests like with the first number (e.g. test_06-09), right now ctest doesn't execute them in the natural order.

I think I fixed everything you pointed out. I will probably come back to the examples and exchange some values for the MuPix10 when I have better ones.

Tests are executed in parallel in the CI, so there is no natural order in running them. But I agree, it's not very nice for ordering. We are planning to make the tests more atomic and eventually move them to their respective modules - then the ordering problem would be largely gone anyway.

[simonspa]

👏