Add steering file to use with raw evio files for 2019

[cbravo135]

No description provided.

[bloodyyugo]

What is this?

[cbravo135]

It is a steering file which uses the apv25 readout crosstalk filter.

[pbutti]

Hi Cam,

I don't fully understand. Why not updating this one?
https://github.com/JeffersonLab/hps-java/blob/master/steering-files/src/main/resources/org/hps/steering/recon/PhysicsRun2019_pass0_recon.lcsim

[cbravo135]

I just wanted it to be clear that this steering file should only be used for running on raw, unskimmed, evio files, so I made it separate. This isn't quite ready btw, still ironing out an issue.

[cbravo135]

After we converge on this steering file, we can also propagate to the other which is OK to run on files that have had events removed or reordered.

[cbravo135]

Ok, running on the sample partitions now, but this PR is more important now because it revealed a memory leak which is fixed by commit 23af46c. I think this is ready to go now, the issue is ironed out.

[cbravo135]

btw, I am not opposed to putting this all in https://github.com/JeffersonLab/hps-java/blob/master/steering-files/src/main/resources/org/hps/steering/recon/PhysicsRun2019_pass0_recon.lcsim, but everyone needs to understand it should only be used for unskimmed evio files.

[normangraf]

This issue seems to have outgrown its original scope. It claims to simply be adding a new steering file, but now includes substantive changes to code. It's best when issues are narrowly targeted and well-described. Can this be split up to address the various issues independently?

[bloodyyugo]

Build ok; tests pass...

[cbravo135]

@normangraf Developing this steering file revealed the memory leak. The fix for the memory leak is a 2 line change, that really is only relevant when this steering file is added. Why split out a 2 line change? IMO this is a waste of time. The hotfix is relevant to this PR.

[cbravo135]

This PR adds a steering file and changes a handful of lines, how is this not a small PR? I can change the title of the PR if @normangraf wants, or open a new PR with a different title so "This issue seems to have outgrown its original scope.". Is the problem that I put it on the iss837 branch, because I can also change the branch name. @normangraf Please provide instructions on the proper procedure, in your opinion, to get these changes in. Please explain where in the HPS policy documents that your particular procedure is more compliant to the text of those documents with respect to this PR.

[pbutti]

Hi All,

I approved this as the changes seem OK, checks/tests passed, no conflicts.
However I think that we should address in the future the steering file proliferation. In particular, if a certain tool can only run on specific type of data (evio, no-skims, or whatever else), it should raise and exception if it runs on the wrong file format/type/events.Shouldn't be left to the operator.
Apart from that, since the fix to the memory leak is just a line change, I'm OK to approve this. I'll wait for Norman tho to bless this PR as he still has some concerns.

[normangraf]

I'm trying to help you become better developers. Best practices dictate that git branches address narrowly-focused issues. It is convenient at the time to simply wrap up multiple disparate issues in one branch, but for long-term maintenance and the debugging of issues which may arise in the future it's easier to have each issue well-documented in a branch and PR.

[normangraf]

I agree with @pbutti's comments on the proliferation of steering files. But let's put that off for another day.

[cbravo135]

I also agree with @pbutti but that is it own whole bag of worms.