XENONnT / straxen

Streaming analysis for XENON
BSD 3-Clause "New" or "Revised" License
20 stars 32 forks source link

Add function to save itp_map InterpolatingMap related dictionary into pickle #1221

Closed dachengx closed 1 year ago

dachengx commented 1 year ago

Before you submit this PR: make sure to put all operations-related information in a wiki-note, a PR should be about code and is publicly accessible

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

Add a function to generate pickle format InterpolatingMap. Codes are mainly copied from https://github.com/XENONnT/nton/blob/dcd7193061778a9a155bfdcb703ef6c19f0b7d60/nton/sim/optical_patterns/optical_simulations/maps.py#L420-L475.

Can you briefly describe how it works?

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

Please include the following if applicable:

Notes on testing

All italic comments can be removed from this template.

coveralls commented 1 year ago

Coverage Status

coverage: 93.553% (+0.008%) from 93.545% when pulling a6739407f03afb5a1f215d505372df379814cddc on itp_map_generator into b3e776f632f224808d823fe924dbde05f1fcdf28 on master.

GiovanniVolta commented 1 year ago

Hey Dacheng, a bit of context for this PR? I have nothing against adding this new function. However, to my understanding, that it is not used to save the interpolated map but rather the not interpolated (aka the histogram from the simulated data). Are you suggesting avoiding the interpolation?

dachengx commented 1 year ago

interpolated

histogram in the codes is just the interpolated map. I will just change the name of histogram to map.

GiovanniVolta commented 1 year ago

interpolated

histogram in the codes is just the interpolated map. I will just change the name of histogram to map.

Sorry, I noticed that you changed the name of the argument, but before, it made more sense to me. What I would prefer is to change the name of the function.

dachengx commented 1 year ago

Sorry, I noticed that you changed the name of the argument, but before, it made more sense to me. What I would prefer is to change the name of the function.

But we are really saving maps, no matter whether the map is from a histogram or somewhere else, right?

GiovanniVolta commented 1 year ago

Mmmm no. The plugin loads the map via straxen.URLConfig and in the protocol, the itm_map keyword is given which calls straxen.InterpolatingMap which then return the interpolator object. In the private repo we have the actual histogram from the GEANT4 simulations. I might be wrong, maybe you or @l-althueser can correct me.

dachengx commented 1 year ago

Mmmm no. The plugin loads the map via straxen.URLConfig and in the protocol, the itm_map keyword is given which calls straxen.InterpolatingMap which then return the interpolator object. In the private repo we have the actual histogram from the GEANT4 simulations. I might be wrong, maybe you or @l-althueser can correct me.

I guess the title of the PR is a bit misleading. The function save_interpolating_map save the map into pickle file, it is not related to URLConfig.

GiovanniVolta commented 1 year ago

No, sorry. Maybe I am too picky, but my point is that we do NOT save the interpolation. Again we save the GEANT4 histogram, which is fed into the interpolation function and gives out an object. If we want this function, a more proper name can be something like save_optical_simualtion_for_straxen

dachengx commented 1 year ago

No, sorry. Maybe I am too picky, but my point is that we do NOT save the interpolation. Again we save the GEANT4 histogram, which is fed into the interpolation function and gives out an object. If we want this function, a more proper name can be something like save_optical_simualtion_for_straxen

But we can use the function in other ways, like S2-xy correction, which is data-driven.

GiovanniVolta commented 1 year ago

So are you saying that we are saving the interpolated S2 map? Could you point me to the file? The only S2 data-driven map I know is from Shenyang and it is a TensorFlow file.

dachengx commented 1 year ago

Yeah, that is also an example.

This function is only to save things in a format InterpolatingMap can understand. It does not only serve optical maps or any specified study.

The purpose to provide a unified function and encourage people to use that is that, pickle can not describe itself because it is binary. I got a lot of confusion when I simply look at a pickle, without source code to generate it. It is better to unify the file generation and formatting into one public function, than people digging into private repos.

GiovanniVolta commented 1 year ago

I agree with your @dachengx, but I am explicitly requesting a name change, please. This name is misleading.

dachengx commented 1 year ago

I agree with your @dachengx, but I am explicitly requesting a name change, please. This name is misleading.

Sorry, do you mean a name change of the PR or of the function?

GiovanniVolta commented 1 year ago

Of the function

dachengx commented 1 year ago

But this function can be applied in various contexts, not only in optical MC.

It only technically wraps a map and corresponding coordination to a dictionary, and saves the dictionary into a pickle. The function does not know what is the kind of the map.

dachengx commented 1 year ago

I did not directly copy the function in nton. I only select the part of pickle saving, to avoid the discussion on the physical context.