Reconfigure the detectors to use global code for the Binary Serializers

[lmalenfant]

The Detectors section of MonteCarlo has a lot of duplicated code and every time we add another detector, the code is duplicated more. The Binary Serializers could be consolidated for the detectors so this would reduce the code duplication.

[hayakawa16]

I am finding I am coding the same code in multiple tests, so I was thinking of optimizing somehow. At first I tried:

        [Test]
        public void Validate_1DRealDetectors_deserialized_class_is_correct_when_using_GetBinarySerializers()
        {

            var detectors = new List<IDetector>();
            var detectorName = "testrofangle";
            detectors.Add(new ROfAngleDetector
            {
                Angle = new DoubleRange(0, 10, 3),
                TallySecondMoment = true, // tally SecondMoment
                Name = detectorName,
                Mean = new double[] { 1, 2, 3 },
                SecondMoment = new double[] { 4, 5, 6 }
            });
            detectorName = "testrofrho";
            detectors.Add(new ROfRhoDetector
            {
                Rho = new DoubleRange(0, 10, 3),
                TallySecondMoment = true, // tally SecondMoment
                Name = detectorName,
                Mean = new double[] { 1, 2, 3 },
                SecondMoment = new double[] { 4, 5, 6 }
            });
            foreach (var detector in detectors)
            {
                DetectorBinarySerializationHelper.WriteClearAndReReadArrays(detector, detector.Mean, detector.SecondMoment);

                Assert.AreEqual(1, detector.Mean[0]);
                Assert.AreEqual(2, detector.Mean[1]);
                Assert.AreEqual(3, detector.Mean[2]);
                Assert.AreEqual(4, detector.SecondMoment[0]);
                Assert.AreEqual(5, detector.SecondMoment[1]);
                Assert.AreEqual(6, detector.SecondMoment[2]);
            }
        }

but IDetector does not include Mean or SecondMoment so not possible. So I am thinking about putting the Assert code in a unit test method. Any thoughts on these optimizations? On the one hand I see that it is great to have an individual, full-contained test for each detector. On the other hand, there is a lot of duplicate code in this unit test file.

[dcuccia]

I think this is the unit test for the detector, right? So, my suggestion is a 1:1 relationship between the .cs test class and the detector (1 "XYZDetectorTests.cs" for each class). And in that case, each detector "knows" about it's own things, so you don't need to represent it with an interface, but instead the implementation type. Yes, it's repetitive, but it segregates different detector tests, and will make those files that are "fine" essentially read-only, while new detectors/tests can be written without disturbing the existing ones (more scalable for new work, new devs, etc). We can make more "helper" static libraries, like var rho = DetectorTestHelper.Generate1DRho() etc.

[lmalenfant]

@hayakawa16 Consolidating test code is a great idea, I think I mentioned this last week in our meeting that if we can create generic code for testing using the data we have, we should go that route rather than duplicating the same code in individual tests.

If you would like to work together on this, let me know. We can probably create another helper method that can be called from each test to avoid the duplication.

[dcuccia]

(and in the case of the 1D, 2D, etc...those should be tests of DetectorIO libraries, with only one example for each "shape"

[hayakawa16]

To respond to @dcuccia, this is the combined test for ROfAngleDetector and ROfRhoDetector. I think you are suggesting not to do this and that the current DetectorIOTests file be separated into one for each detector? If so would that detector test also test methods like Normalize or just GetBinarySerializers?

[dcuccia]

@hayakawa16 all tests relevant to that detector's performance and behavior (and nothing else).

[hayakawa16]

Got it. Let me discuss with @lmalenfant how we organize these types of single layer tests versus the integration tests. In the meantime I will continue filling out the tests in DetectorIOTests so that I can verify new code, and I can separate later.

[dcuccia]

Sounds good!

[hayakawa16]

I'm half way there with the unit tests! I see that there have been multiple topics brought up in this issue. I have created a list of the order I plan to proceed (this is up for discussion):

1. complete unit tests of all detectors GetBinarySerializers (all in one file)
2. update single value detectors to use empty array instead of null and write corresponding GetBinarySerializers unit tests (I tried this today and had trouble, may need help)
3. write unit test for BinaryArraySerializerFactory (again will gladly welcome any help)
4. separate detector unit tests into separate files ---- at this point merge in branch (option: could merge after 3)
5. analyze PopulateFromEnumerable and possibly update application to be column-major
6. reorganize Vts.Test to Vts.Test.Unit, Vts.Test.Integration I think the last two are separate topics are beyond this issue and should be solved on other issue branches. Let me know if I omitted anything. Just trying to lay out steps for myself.

[lmalenfant]

This looks good @hayakawa16 although I think 2. could be further down the list because this is part of a larger cleanup effort. Plus the detectors that contain this code do not have the serialization code and are not being modified with this change.

Since this is already a huge change we need to keep these changes to a minimum because they will be easier to review in the PR.

I would choose the option to merge after 3.

[lmalenfant]

@hayakawa16 would you like me to write the unit tests for BinaryArraySerializerFactory?

@dcuccia I was able to look at your Gist finally and I like it, if we write the unit tests for the code as is, we can switch it for the new code and validate that it still works the same way.

[dcuccia]

Sounds good! :)

[hayakawa16]

@lmalenfant that would be super great!

[hayakawa16]

I have finished updated all 72 detectors with the new GetBinarySerializers code and written unit tests for them all (included testing what results if TallySecondMoment is set to true and false using neat nunit [TestCase(true)] and [TestCase(false)] attributes that Lisa used in her unit tests for BinaryArraySerializerFactoryTests). These tests are currently in a single file. I plan to break each test into a separate file named by the detector it is testing. For example, ROfRhoDetectorTests.cs. The GetBinarySerializers method will be the only test for ROfRhoDetector in this file, however in the future we can add other unit tests to this file for the other methods in ROfRhoDetector class.

I plan to create a folder under Vts.Test named "Unit". Then in that folder, a subfolder named "MonteCarlo", then a subfolder in that named "Detectors". Then when I separate the unit tests into the separate files, I will put them in this "Detectors" folder and give the tests the namespace Vts.Test.Unit.MonteCarlo.Detectors.

Let me know if you have any comments or questions about this plan.

[lmalenfant]

@hayakawa16 That sounds good to me.