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

Run2021 Merge #909

Merged
merged 11 commits into from
Apr 28, 2022
Merged

Run2021 Merge #909

merged 11 commits into from
Apr 28, 2022

Conversation

pbutti
Copy link
Contributor

@pbutti pbutti commented Apr 19, 2022

Time to bring back run2021 into master.

@pbutti pbutti changed the title Run2021 Merge - WIP Run2021 Merge Apr 19, 2022
Copy link
Collaborator

@cbravo135 cbravo135 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been using this branch for months, given multiple talks using it, including the recent work I did aligning the 2021 SVT.

@JeremyMcCormick
Copy link
Member

JeremyMcCormick commented Apr 19, 2022

Has the full MC chain been checked with this on a few of the new detectors and maybe a couple of the old ones?

Might want to do this as it would verify that the LCDD and compact don't have a geometry mismatch.

@cbravo135
Copy link
Collaborator

I don't think it is needed to check the MC chain for multiple detectors for this PR. The change that would potentially change stuff wrt the lcdd/compact already happened in #903. The one thing here that I didn't realize was in the branch are changes from TongTong for the DAQ configuration work he did. I do have some comments on how that was done, mostly that there is a big block of else if statements to choose the correct resource for the correct run number. This should have been done using the database, as this sort of issue is exactly what the database system was designed to solve.

@tongtongcao
Copy link
Contributor

tongtongcao commented Apr 20, 2022

I don't think it is needed to check the MC chain for multiple detectors for this PR. The change that would potentially change stuff wrt the lcdd/compact already happened in #903. The one thing here that I didn't realize was in the branch are changes from TongTong for the DAQ configuration work he did. I do have some comments on how that was done, mostly that there is a big block of else if statements to choose the correct resource for the correct run number. This should have been done using the database, as this sort of issue is exactly what the database system was designed to solve.

For a DAQ configuration, it includes some parameters, but not much like gain or pedestals or else for Ecal/Hodo/SVT channels.
Currently, we already have parsers to parse DAQ configuration for each experiment run, and have a system to reach parameters based on the parsers.
For each experiment, there are only several versions for DAQ configuration. So we just need small space to save DAQ configurations in resource.
Certainly, we can use DB to store parameters. But it will be a big task.
Besides development of complicates tables for DAQ configurations (many details in DAQ configuration), I can image that all stuffs related to DAQ configuration in hps-java need to be modified, not just for 2021, but also for 2019 and 2016.
I personally don't think that it is worthy to take too much energy on developing DB for DAQ configuration, and just for saving so small space expense.
For configurations in hps-java, DB is an option, but resource is also a choice for small space expense.

@@ -329,6 +339,62 @@ else if((runNumber >= 10637 && runNumber <= 10644) || (runNumber <= 10647 && run
return "hps_v12_1";
else if(runNumber >= 10716 && runNumber <= 10718)
return "hps_v13_FEE";

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear for TongTong, my comment was not about the data from the config files being a resource. My comment was about this particular block of code being a mess. What this code achieves can be more cleanly achieved via the db, by making a table that holds the names of each of the config file resources that could be used and making entries in the conditions db to associate each one with the appropriate run numbers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds not a big job. Let's put it into to-do list.
The current code works. I suggest to merge the branch firstly, and then open an issue to do update later.

Copy link
Contributor

@tongtongcao tongtongcao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked on the branch for a while. No issues happened. So I approve the PR.

@cbravo135 cbravo135 merged commit d7fabae into master Apr 28, 2022
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.

4 participants