Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'electron_diffusion_cte' variable from CMT #1025

Merged
merged 2 commits into from Apr 28, 2022

Conversation

ricmperes
Copy link
Contributor

@ricmperes ricmperes commented Apr 21, 2022

What does the code in this PR do / what does it improve?

The longitudinal diffusion constant value was added to the CMT but not to the single_value_corrections list. Without it, straxen.get_correction_from_cmt(run_id', ('electron_diffusion_cte', version, detector)) leads to an error.
Issue in code found by @l-althueser.

This PR adds support for straxen.get_correction_from_cmt to fetch the diffusion constant from CMT.

Can you briefly describe how it works?

Main code stays unaltered, 'electron_diffusion_cte' added to the single_value_corrections list.

Can you give a minimal working example (or illustrate with a figure)?

straxen.get_correction_from_cmt('17895', ('electron_diffusion_cte', 'ONLINE', 'nT'))

The longitudinal diffusion constant value was added to the CMT but not to the `single_value_corrections` list. Without it, `straxen.get_correction_from_cmt(run_id', ('electron_diffusion_cte', version, detector))` leads to an error.
Issue found by @l-althueser.
@coveralls
Copy link

coveralls commented Apr 21, 2022

Coverage Status

Coverage increased (+0.04%) to 93.807% when pulling cd5f6b5 on diff_const_addition into 581f120 on master.

@ricmperes ricmperes marked this pull request as ready for review April 21, 2022 12:47
@l-althueser
Copy link
Contributor

I would give green light for this as this is exactly the workaround that I provided. Thanks @ricmperes!

Maybe some CMT expert can comment if we miss some important spot in the code, @ahiguera-mx ?

@ahiguera-mx
Copy link
Contributor

As far as I know this is not use anywhere in straxen, right? but we retrieve the information via CMT therefore this patch.
The electron diffusion cte is use on cutax, right?

@ricmperes
Copy link
Contributor Author

Right. It's only used in cutax for the S2Width cut to make it time-dependent and/or directly dependent to CMT instead of having hardcoded values (here in PR).

@WenzDaniel
Copy link
Contributor

Tagging https://github.com/XENONnT/cutax/pull/166/files for completeness.

@JoranAngevaare JoranAngevaare merged commit 9a6c29d into master Apr 28, 2022
@JoranAngevaare JoranAngevaare deleted the diff_const_addition branch April 28, 2022 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants