Closed Yero1990 closed 6 years ago
MarkKJones
commented
6 years ago For a small fix, I prefer just to have a flag. It is hard enough to have the experiments keep track of their parameter files. Getting experiments to separately keep up a fork of hcana for one little fix seems not possible.
sawatzky
commented
6 years ago This kind of thing gives me the creeps as far as code quality and maintainability goes... Hardcoded magic numbers and even generically named flags like this may be easy in the short term, but it's embedding experiment-specific cruft into the general analyzer that will be carried forward for a long time...
If this must live on in the main code base in the short term then, at minimum, the flag should be named something fairly specific like Spring18_ROC3_hotfix (or whatever) and have a commit message that clearly explains reason for the hack and the run period for which it is valid.
I hope we can come up with a better way of managing this kind of thing in the long run. I know from experience that burned-in special case cruft has a way of causing problems in the future. (People copy-paste analysis scripts and leave it enabled inappropriately. Hardware maps change, but the hardcoded indices don't, etc.)
For example, one group in Hall A lost months chasing some hardcoded (wrong) 'optics fixes' that got embedded into a module instead of being put in the module's config file where they belonged.
On a related note, Eric and I found at least one other special-case hardcode just by chance this afternoon. That one wasn't even protected/enabled/annotated by a flag.
-- Brad
On Fri, 24 Aug 2018, Mark K Jones wrote:
For a small fix, I prefer just to have a flag. It is hard enough to have the experiments keep track of their parameter files. Getting experiments to separately keep up a fork of hcana for one little fix seems not possible.
-- Brad Sawatzky, PhD brads@jlab.org -<>- Jefferson Lab / Hall C / C111 Ph: 757-269-5947 -<>- Fax: 757-269-5235 -<>- Pager: brads-page@jlab.org The most exciting phrase to hear in science, the one that heralds new discoveries, is not "Eureka!" but "That's funny..." -- Isaac Asimov
hansenjo
commented
6 years ago Looks like what you need here is an array of per-wire sigma values.
In the Hall A VDC code, we represent each wire with its own object. (About 750 small objects in memory are manageable.) In this way, we can assign parameters on a per-wire basis, or for arbitrary groups of wires. But our experiments ended up not caring much about such subtleties, probably because the VDCs are quite resistant to miscalibration, unlike your chambers. So we never implemented a convenient general-purpose database reader for per-wire parameters. That's what you'll have to do here if you want to do it right.
Enabling special code with a flag is probably OK as a quick fix, but I'd rename the flag to something more descriptive, e.g. fFixSpring2018TDCBadPerWiregroupSync. Also, I'd put loud and clear FIXMEs around such code. And start working on adding optional per-wire parameters to the database.
Separate code branches may or may not be a good idea. Yes, you can put such kludges in with more impunity, but you'll also fall out of sync with code fixes in the mainline branch. Then again, not all of those fixes may be something you want for your data. So you quickly end up with either stale code or a maintenance headache.
The solution that I have found to work best in Hall A are experiment-specific libraries where people inherit from standard detector classes and implement their non-trivial changes in the relevant virtual functions. In this way, it is always clear where and what the experiment-specific code is, and the experiment can still benefit from general improvements and bug fixes in the core part of the analyzer (the parts they didn't oevrride).
Ole
MarkKJones
commented
6 years ago Ok. I will close this and ask Carlos to implement a per wire sigma. It seems like a lot of work. Then the experiment specifics will be in the parameter file.
hansenjo
commented
6 years ago Yes, it is a bit of work, but it only needs to be done once, and all future experiments will be able to tweak parameters for individual wires or wire groups.
Maybe there are other parameters that could be set this way as well. An on/off flag to mark bad wires might be useful too.
Yero1990
commented
6 years ago Ole,
I remember Hall C had a parameter to specify bad wires, but these was removed at some point. Maybe we can implement this again.
Carlos
On Fri, Aug 24, 2018 at 9:10 PM, Ole Hansen notifications@github.com wrote:
Yes, it is a bit of work, but it only needs to be done once, and all future experiments will be able to tweak parameters for individual wires or wire groups.
Maybe there are other parameters that could be set this way as well. An on/off flag to mark bad wires might be useful too.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/JeffersonLab/hcana/pull/395#issuecomment-415918936, or mute the thread https://github.com/notifications/unsubscribe-auth/AQD76umY_tfrhyCSgyPJyotHYMBE30Wpks5uUKQPgaJpZM4WLpC1 .
This kind of 'special case' fix should not really be pushed into the main analyzer code.
It would be best to carry this kind of thing in an experiment-specific branch/fork.
-- Brad
On Fri, 24 Aug 2018, Yero1990 wrote: