SBNSoftware / sbndcode

11 stars 44 forks source link

Feature/bethanym hit dumper pr #475

Closed bethmccusker closed 2 weeks ago

bethmccusker commented 4 months ago
bear-is-asleep commented 3 months ago

Whilst I agree in principle @bear-is-asleep to some of your comments I'd like to point out the following things:

  • The current fcls here are quite messy anyway, the naming doesn't make much sense and neither do the defaults.
  • For example, PDS seems to arbitrarily run as default whilst TPC and CRT not?
  • None of them really reflect a realistic workflow going forward.

And suggest the following path:

  • Beth's PR is merged as is.
  • In the "data PR" that I've promised you early next week I will include a tidying up of the fcls in this folder, which you can then review and improve upon in that PR?

Hi Henry, sorry for the late response. I don't think I commented on the naming conventions but they seem fine to me (you didn't change the names of anything I don't see). I get hitdumper will be rapidly changing, but we should nonetheless not merge arbitrary changes just because "it's messy anyways". I agree the defaults don't make sense, but if a future contributor sees they were changed in a release, it may be confusing. Since we're taking commissioning data, it'd be great to update the defaults to things we have data for and work, namely the PDS reco and now CRT track reco. However, I don't expect you or Beth to make these changes, I can probably make these changes myself.

That being said, I'm happy to approve these changes and I understand your perspective, I don't want to labor you two over a few arbitrary fcl changes! I'm just happy the CRT reco is in. Just saying we need to treat our code with love.

bear-is-asleep commented 1 month ago

Is this PR still being worked on?

henrylay97 commented 3 weeks ago

Okay I've provided some tidy up of this to avoid changing default parameters etc.

@bethmccusker can you check that you're happy that the two run fcls that you created still run happily on your files and give you everything you need?

bear-is-asleep commented 2 weeks ago

trigger build LArSoft/lar*@LARSOFT_SUITE_v09_91_02

FNALbuild commented 2 weeks ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

FNALbuild commented 2 weeks ago

:heavy_check_mark: CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

FNALbuild commented 2 weeks ago

:x: CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard

:rotating_light: For more details about the failed phase, check the build SBND phase logs

parent CI build details are available through the CI dashboard

FNALbuild commented 2 weeks ago

:warning: CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof -- details available through the CI dashboard

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

parent CI build details are available through the CI dashboard