Feature/112 reconfigure the detectors to use global code for the binary serializers 2

[hayakawa16]

Creating a draft PR so it goes through all of the checks. Will add reviewers next week.

[lmalenfant]

@hayakawa16 The quality gate passed!

There are 11 new code smells, we can't fix them all (some are cognitive complexity with the factory code) but some of them can be fixed.

[lmalenfant]

In every detector unit test file except one (ReflectedDynamicMTOfXAndYAndSubregionHistDetectorTests) there was an extra line between the comment and the test method. This visually disconnects the comment from the method. I commented on the first few files but to avoid duplicate comments, added it here.

[hayakawa16]

Thank you for your very detailed review! I made code changes for all suggestions, retested and pushed.

[lmalenfant]

I have added a few more comments where there is a line feed before the end of an object definition, it is not intentional whitespace and I think it could result in a developer putting future code in the wrong place.

[lmalenfant]

Looks good! I marked all change requests as resolved.

[hayakawa16]

Thanks Lisa! I will await dcuccia's review before proceeding.

[hayakawa16]

Just wanted to post that I took an infile that I ran with master on 10/12/23 and reran it using this branch. The results are identical! It was a sanity check suggested by lmalenfant and gives us more confidence in our code modifications.

[dcuccia]

Looks great - thanks for all the hard work that went into this. My one comment that we should consider changing before merging is that we update the names of the test methods. 'Validate_deserialized_class_is_correct_when_using_GetBinarySerializersis less accurate thanValidate_deserialized_binary_arrays_are_correct_when_using_GetBinarySerializers`.

My comments regarding things we might consider for the future are:

1. That we refactor the test projects into separate Integration and Unit projects (and as part of that, generally better align test names to the classes/class members they're testing).
2. That we pare down Vts.Test.MonteCarlo.DetectorIOTests significantly, now that all detectors' serializers' behavior/correctness are already covered by these new unit tests (don't need duplicate checks - wastes time and increases software maintenance load). Remaining work would probably be to refactor what remains to unit tests that test the detector-specific JSON serialization/deserialization for the non-array properties.
3. Similarly, to refactor test classes in the Vts.Test.MonteCarlo.Detectors into their Unit counterparts, if feasible - that way we can declare + test the intended behavior of the specific detector without relying on executing a Monte Carlo simulation (i.e. correct tally accumulation and normalization, e.g. focus on correctness of code, not accuracy of MC).
4. Once 1 & 2 are done, we would be left with tests that test the accuracy of the Monte Carlo for those detectors, which would be probably best labeled as integration tests. (An implementation detail, but a potential way to do that rapidly be to use an in-memory post-processor database, so you don't have to "run" an MC for each detector, just walk through the photon data points).

Perhaps we could create a couple follow-on placeholder tickets (to work on when appropriate) that documents our desire to do the above?

[hayakawa16]

dcuccia, thank you for your review! I can easily modify the unit test names.

I like your ideas for future work. It always bothered me how many times a MC simulation is executed during the course of running the unit tests. If this could be optimized somehow, that would be great! In addition to your list, in our discussion on this issue we uncovered the need to analyze PopulateFromEnumerable and possibly update application to be column-major. Creating place holder issues for these features sounds good. Would you like to create them?

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

100.0% Coverage
8.9% Duplication