next-exp / IC

6 stars 70 forks source link

Reorganization of the reco folder #885

Closed paolafer closed 1 month ago

paolafer commented 2 months ago

I would like to complete the job done in PR #707 but never finished, meaning, reorganizing the reco folder. At the moment this folder contains very different functions and some of them are not related to reconstruction, from my point of view.

My proposal is the following:

  1. sensor_functions and spe_response are part of the MC simulation of the events, rather than the reconstruction, therefore they should go to a dedicated folder (maybe a new mcsim?).
  2. tbl_functions are very generic functions to deal with tables, so they could go to core.
  3. I'm not sure about wvf_functions; they deal with waveforms, so they are part of the reconstruction in a way.
  4. Same about the two calib files. Calibration can be considered part of the reconstruction, so they can stay here.

Opinions?

gonzaponte commented 1 month ago

Thank you for addressing this.

  1. Agreed. I would merge it with the detsim directory and name it sim or maybe leave the same name. I would also include other parts of the code such as the noise sampler in core/random_sampling which has been living there for a while.
  2. Agreed.
  3. I would also keep this one in reco
  4. I would move these to a new calib folder. The code for the Kr maps could also go in there.
paolafer commented 1 month ago

Maybe I'm missing something here, but it looks like spe_response is not imported anywhere in IC, except for its own test file. Is that correct?

gonzaponte commented 1 month ago

You are right. I believe these were functions used in external calibration programs. The same used to happen with fit_functions in core. We should check if someone is still using them.

@joshgrocott, are you using functions from spe_response.py?

joshgrocott commented 1 month ago

Hi Gonzalo,

Yes, namely the calibration scripts use scaled_dark_pedestal. Is there a problem?

Regards, Josh


From: Gonzalo @.> Sent: Tuesday, July 23, 2024 10:24 AM To: next-exp/IC @.> Cc: Joshua Grocott @.>; Mention @.> Subject: Re: [next-exp/IC] Reorganization of the reco folder (Issue #885)

You are right. I believe these were functions used in external calibration programs. The same used to happen with fit_functions in cope. We should check if someone is still using them.

@joshgrocott [github.com]https://urldefense.com/v3/__https://github.com/joshgrocott__;!!PDiH4ENfjr2_Jw!AsHM1NHMcxSwrX3BqVnSPe0A6k-SuxmlHhe7JtnsK3fk8v11v-nz3Wv8_Kc9_qYhl_kdb7EaJfV0ocQuYKg4XBMonxbZZqEv1Drosvy9hUg$, are you using functions from spe_response.py?

— Reply to this email directly, view it on GitHub [github.com]https://urldefense.com/v3/__https://github.com/next-exp/IC/issues/885*issuecomment-2244701163__;Iw!!PDiH4ENfjr2_Jw!AsHM1NHMcxSwrX3BqVnSPe0A6k-SuxmlHhe7JtnsK3fk8v11v-nz3Wv8_Kc9_qYhl_kdb7EaJfV0ocQuYKg4XBMonxbZZqEv1DroHaNe2Ww$, or unsubscribe [github.com]https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A333VGDF4L7NHST5W2D5PALZNYONFAVCNFSM6AAAAABJ7VADUWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBUG4YDCMJWGM__;!!PDiH4ENfjr2_Jw!AsHM1NHMcxSwrX3BqVnSPe0A6k-SuxmlHhe7JtnsK3fk8v11v-nz3Wv8_Kc9_qYhl_kdb7EaJfV0ocQuYKg4XBMonxbZZqEv1Dro92YJM0U$. You are receiving this because you were mentioned.Message ID: @.***>

gonzaponte commented 1 month ago

Is there a problem?

Nope. Just notice that when this is merged (and you update your local IC) you will have to adjust the imports in your scripts.

gonzaponte commented 1 month ago

Also, this indicates that spe_response belongs to calib, not detsim.

bpalmeiro commented 1 month ago

As the plan, I'd say, is to eventually incorporate said external process into IC, we can think of incorporating it into the calibration folder so it's kinda self-contained. Maybe we can even consider changing the name to accommodate.