search

VERITAS-Observatory / V2DL3

VERITAS (VEGAS and Eventdisplay) to DL3 Converter
BSD 3-Clause "New" or "Revised" License
8 stars 1 forks source link

missing parameters in `v2dl3` script #133

Closed kpfrang closed 2 years ago

kpfrang commented 2 years ago

I'm not able to run the v2dl3 script with the latest version. I believe this is due to the parameter fuzzy_boundary that is added to the v2dl3_for_Eventdisplay.py script but not in v2dl3. However, I believe that it is required here:

https://github.com/VERITAS-Observatory/V2DL3/blob/3b4691cbb3a06805b722e494d4ae84ce7a866dd4/pyV2DL3/eventdisplay/fillRESPONSE.py#L56

Probably this parameter should be added to the v2dl3 script as well, right? The same also applies to the force_extrapolation parameter I believe.

sona-patel commented 2 years ago

thanks @kpfrang, you are right! The converter will not work with v2dl3.py after PR #131 having two additional command line paramters. Also, note that currently, for running the converter on eventdisplay anasum stage files, we are using v2dl3_for_Eventdisplay.py. v2dl3.py is obsolete for eventdisplay (see here).

  1. Are you using the converter on vegas files?
  2. If not, then do have any specific reasons for using v2dl3.py instead of v2dl3_for_Eventdisplay.py
kpfrang commented 2 years ago

Thanks for the explanation. No, I'm running it for EventDisplay. I just didn't understand that v2dl3_for_Eventdisplay.py replaces v2dl3.py in the EventDisplay case. I would suggest renaming v2dl3.py to v2dl3_for_vegas.py to avoid this confusion in the future.

GernotMaier commented 2 years ago

The hope is that we are able to unify those scripts again, this is why we didn't do the renaming. It is clearly discussed in the README on the main page.

Do you still think we should rename it? Of course could always go back, but renaming it would confuse all the colleagues using vegas (who should not be affected by changes we do on the eventdisplay side)

kpfrang commented 2 years ago

I don't have a strong opinion about that. I just suggested it in order to be explicit with the names.

I think, ANALYSIS.anasum_parallel_from_runlist.sh still uses v2dl3 when it generates the v2dl3 script. So I expected that this is still working. But you are right, the README is clear about it. Just didn't know that I need to reread it.

Should v2dl3_for_Eventdisplay be added to the setup.py? https://github.com/VERITAS-Observatory/V2DL3/blob/789b3e0bdffbab0f58d95b90cd1b0f323a7a7431/setup.py#L19-L24

And how about the ANALYSIS.anasum_parallel_from_runlist.sh script?

GernotMaier commented 2 years ago

I don't have a strong opinion about that. I just suggested it in order to be explicit with the names.

Completely makes sense your suggestion, but I am hesitating changing names which impacts the vegas side.

I think, ANALYSIS.anasum_parallel_from_runlist.sh still uses v2dl3 when it generates the v2dl3 script. So I expected that this is still working. But you are right, the README is clear about it. Just didn't know that I need to reread it.

Didn't have ANALYSIS.anasum_parallel_from_runlist.sh on my radar. Obviously a dependency outside of V2DL3, which is hard to track.

All what it does is to write a bash script with the converter command? Maybe you or Sonal can adjust it.

Should v2dl3_for_Eventdisplay be added to the setup.py?

https://github.com/VERITAS-Observatory/V2DL3/blob/789b3e0bdffbab0f58d95b90cd1b0f323a7a7431/setup.py#L19-L24

setup.py is only needed when running pip install - at this point we don't need to do this to run v2dl3_for_Eventdisplay, all what needs to be done is to to set the python path (note that you need to run with python pyV2DL3/script/v2dl3_for_Eventdisplay.py).

We need to discuss if we want a pip installation or maybe even use a package manager like conda to run the converter.

And how about the ANALYSIS.anasum_parallel_from_runlist.sh script?

GernotMaier commented 2 years ago

I assume that this issue has been solved (or should be one for the Eventdisplay script repository)