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

added sbnd recomb and diffusion fcls #366

Merged
merged 16 commits into from
Jul 13, 2023
Merged

added sbnd recomb and diffusion fcls #366

merged 16 commits into from
Jul 13, 2023

Conversation

ibsafa
Copy link
Contributor

@ibsafa ibsafa commented Jul 10, 2023

No description provided.

@ibsafa ibsafa requested a review from jzennamo July 10, 2023 18:11
@fjnicolas
Copy link
Contributor

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_75_02 SBNSoftware/sbncode@v09_75_02 SBNSoftware/sbnanaobj@v09_21_02 SBNSoftware/sbnobj@v09_17_03

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for c7:prof -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for SBND Succeeded on slf7 for e20:prof -- details available through the CI dashboard

parent CI build details are available through the CI dashboard

@fjnicolas fjnicolas requested review from sjgardiner and cerati July 11, 2023 15:47
@fjnicolas
Copy link
Contributor

@sjgardiner @jzennamo @cerati can you have a look/sign the PR?

@henrylay97
Copy link
Member

As discussed with @fjnicolas offline. This is a potentially temporary update to this PR. Decision on wirecell inclusion are still being made and so further updates may be needed this afternoon.

lynnt20 added 2 commits July 12, 2023 16:00
- adds a detsim fcl that **turns off** 1d tpc sim, and only runs crt+pds sim
- adds a reco1 fcl that takes wc 2d sim as input
@lynnt20
Copy link
Contributor

lynnt20 commented Jul 12, 2023

Just added two fcls that:

  1. in detsim stage, ignore the daq producer (turning off 1D TPC simulation). Run this assuming that wirecell_tbb_sim_sbnd.fcl is performing 2D TPC simulation.
  2. in reco1 stage, use simtpc2d:daq as input to caldata and fasthit

@fjnicolas
Copy link
Contributor

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_75_02 SBNSoftware/sbncode@v09_75_02 SBNSoftware/sbnanaobj@v09_21_02 SBNSoftware/sbnobj@v09_17_03

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for e20:prof -- details available through the CI dashboard

@FNALbuild
Copy link
Collaborator

✔️ CI build for LArSoft Succeeded on slf7 for c7:prof -- details available through the CI dashboard

@fjnicolas fjnicolas mentioned this pull request Jul 13, 2023
@FNALbuild
Copy link
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e20:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@FNALbuild
Copy link
Collaborator

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for c7:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

@henrylay97 henrylay97 self-requested a review July 13, 2023 17:12
Copy link
Member

@henrylay97 henrylay97 left a comment

Choose a reason for hiding this comment

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

Following conversations and offline tests with @lynnt-uchicago @ibsafa and others we're happy that this PR is ready to go and by extension everything needed for the release is in place!

@fjnicolas fjnicolas merged commit c2d9272 into develop Jul 13, 2023
Comment on lines -126 to -133
outputCommands: [ "keep *_*_*_*"
# Drop the SimEnergyDeposits made by LArG4
, "drop sim::SimEnergyDeposits_largeant_*_*"
# Drop the IonAndScint w/ SCE offsets applied
, "drop *_ionandscint__*"
# Drop LArG4 AuxDetHits, now replaced by AuxDetSimChannels
, "drop sim::AuxDetHits_*_*_*"
]
Copy link
Member

Choose a reason for hiding this comment

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

Hi! Sorry I saw this PR was merged already, but I just noticed that this PR changes the standard g4 fcl. Is this intentional? This way, it's keeping all data products, with many copies of SimEnergyDeposits. @fjnicolas @lynnt-uchicago @henrylay97

Copy link
Contributor Author

@ibsafa ibsafa Jul 13, 2023

Choose a reason for hiding this comment

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

hi! yes, the reason is that g4_drops.fcl was created for the lite version. it was decided that sim energy deposits would be dropped as part of the lite workflow but should be available by default since a few people have needed those sim energy deposits recently

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying @ibsafa! LArG4 makes three copies of SimEnergyDeposits, two of them can be dropped without loss (but people can correct me!). The reason why all three were dropped was a mistake, and was fixed by @fjnicolas with PR #368. My suggestion would be to keep that block above in the standard fcl, and add more drops commands in the g4_drops.fcl for production purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@marcodeltutto thanks for the suggestion! A conservative solution was finally adopted prioritizing having a working tag for the workshop production. I also agree we are probably keeping "to much" with the current solution and we will revisit it after the workshop. In any case, we think a major refactoring of the fhicls is really needed at this point. I'm opening a issue to keep track of it! #374

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good @fjnicolas, thanks! I agree a refactoring is really needed

@henrylay97 henrylay97 deleted the isafa/detvar branch January 12, 2024 08:47
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.

6 participants