R3BRootGroup / R3BRoot

Framework for Simulations and Data Analysis of R3B Experiment
https://github.com/R3BRootGroup/R3BRoot/wiki
GNU General Public License v3.0
18 stars 102 forks source link

Revert naming scheme for trigger leading time #1064

Closed Morningrize closed 3 months ago

Morningrize commented 4 months ago

This PR reverts the naming scheme for the leading edge trigger time in NeuLAND.

If this affects you and your workflow, please comment and state your preference for the naming, and I would ask the maintainers to accept or discard this PR depending on the outcome of the discussion.

After the TAMEX firmware update, we updated the unpacker mapping with the signal for the trailing edge. We used the same naming scheme as other TAMEX systems (LOS and TOFD). This led to the following change NN_TRIG -> NN_TRIGL. This change was introduced in the R3BRoot_fast branch to the NeuLAND reader as well and requires updating unpackers from previous experiments, but has since been merged to the dev branch as well. This PR reverts it.

If accepted, it will be accompanied by corresponding changes in the s091_s118 unpacker. If declined, it will be accompanied by necessary changes in unpackers for other experiments. In either case, files that have already been unpacked will not need to be unpacked again, since the change only affects the reader. While we are encouraged to use the most recent versions of our software for analysis, I understand that divergences may occur due to individual workflows and practices and that changes in the naming scheme could potentially be disruptive to some.

Please comment. Best regards, Andrea H


Checklist:

YanzhaoW commented 4 months ago

Thanks very much for the PR.

I think the main problem of using new variable names is the incompatibility for the people who use the older version of unpacker (upexps) for older experiments. What we could do is to enforce people to always use the latest version of unpacker just like we suggest people to use latest version of R3BRoot. But unfortunately the git status of the unpacker in /u/land/fake_cvmfs/10/cvmfs_fairroot_v18.8.0_fs_nov22p1_extra/upexps is in Limbo.

This is git log:

commit 2b42821d8fd2cbb722468eadbab8a4c751941969 (HEAD -> 202402_s091_s118)
Author: A land user who did not set git config --local <land@gsi.de>
Date:   Sat Mar 16 09:23:47 2024 +0100

    Added right mapping for trloii and califa sampler

commit 49ce454f5f4206db366f4ab7f8d7178c9bcd9bc7 (origin/202402_s091_s118)
Author: A land user who did not set git config --local <land@gsi.de>
Date:   Thu Feb 22 18:05:35 2024 +0100

    changes to the frs mapping to now only have Cave C proton start and S2

commit 165ae7b6d0693e048f64ec6db1837c9342040d2d
Author: A land user who did not set git config --local <land@gsi.de>
Date:   Wed Feb 14 02:11:35 2024 +0100

    Add califa mapping files

commit 5285eb5869c915fa9613dcd06b4d0409b311ff56
Author: A land user who did not set git config --local <land@gsi.de>
Date:   Wed Feb 14 01:48:11 2024 +0100

    Night shift commit of s091/s118 related stuff

And git status:

On branch 202402_s091_s118
Your branch is ahead of 'origin/202402_s091_s118' by 1 commit.
  (use "git push" to publish your local commits)

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

    modified:   202402_s091_s118/202402_s091_s118_user.cc
    modified:   202402_s091_s118/mapping_CALIFA.hh
    modified:   land_common/land_vme.spec

Untracked files:
  (use "git add <file>..." to include in what will be committed)

    201810_s444/201810_s444.spec
    201902_s444/mapping_pspx_r3broot_new.hh
    202103_s455/202103_s455_part2.spec
    202105_s494/grep
    202205_s509/202205_s509
    202205_s522/202205_s522
    202302_eng/202302_eng
    202302_eng/mapping_fibsixty.C
    202302_eng/trig.out
    202302_eng/ucesb.out
    202302_eng/unused/
    202402_s091_s118/202402_s091_s118
    202402_s091_s118/mapping_CALIFA_bug_in_CEPA.hh
    202403_s122/
    asyeos_s122/
    build_new.sh
    califa/gamma_mapping_26_02_2024_CALIFA.hh
    califa/gamma_mapping_26_03_2024_CALIFA.hh
    califa/proton_mapping_26_02_2024_CALIFA.hh
    califa/proton_mapping_26_03_2024_CALIFA.hh
    jun16/jun16_C.spec
    jun16/jun16_U.spec
    jun16/jun16_Xe.spec
    jun16/jun16_ptof.spec
    structify_ext_standalone.bash

Here we can see some people didn't follow the rule and since we are all using land account, we have no idea who they are. The latest version of upexps may not even work. Therefore, before we enforce people to use the lastest version of upexps, we need to clean up the local git repo and push it to its remote repo. Local repo should be closed to any change and every change to unexps should go through PRs, such that we know exactly who and when did the change.

I'm all for the consistent naming scheme. But before cleaning up the git repo and ensuring the latest version is working, it's better to keep the names same with the old experiments and not to break the workflow of other people.

@igasparic @inkdot7 @jose-luis-rs @hapol

nmozumdar commented 3 months ago

Thank you for the PR, and I am in complete favor of reverting the changes especially because it affects our experiment S509 and S522. We are at a stage where most of our data is calibrated and unpacked and changing upexps now so long after the experiment would be only detrimental. Especially because of intertwined R3BRoot versions used this far into the analysis.

Changing and finalizing the unpacker of newer experiments when they are just starting calibration I think is a more reasonable approach.

jose-luis-rs commented 3 months ago

I agree, if the leading times are still needed you could create a new reader for the last experiments. Thanks