Reconfigure the detectors to use global code for the Binary Serializers

[lmalenfant]

There are currently 72 detectors in the Monte Carlo code and new ones are added periodically, there are currently 2 new detectors being developed. When creating new detectors, the issue is that they are often modelled on an existing detector and a lot of the code get's duplicated. One of the areas of duplication is the array serialization. I am proposing that we make this code parameterizable and then implement it just once for each scenario.

The 72 detectors fall into one of 17 categories:

1. Single value detectors: no binary serializer
2. 1D Real detectors with (Count-1):
3. 1D Complex detector with (Count)
4. 2D Real detectors with (Count-1, Count-1)
5. 2D Real detectors with (Count-1, Count-1) plus other binary output
6. 2D Complex detectors (Count, Count-1)
7. 2D Complex detectors (Count,Count-1) plus other binary output
8. 2D Complex detectors (Count-1, Count)
9. 3D Real detectors with (Count-1, Count-1, Count-1)
10. 3D Real detectors with (Count-1, Count, Count-1)
11. 3D Real detectors with (Count-1, Count-1, Count-1) plus other binary output
12. 3D Complex detectors with (Count-1, Count-1, Count)
13. 3D Complex detectors with (Count, Count-1, Count-1)
14. 4D Real detectors with (Count-1, Count-1, Count-1, Count-1)
15. 4D Real detectors with (Count-1, Count-1, Count-1, Count)
16. 4D Complex detectors with (Count-1, Count-1, Count-1, Count)
17. 5D Real detectors (Count-1, Count-1, Count-1, Count-1, Count-1)

In general there are 2 binary serializers for each detector and some detectors have additional binary output, these outputs would be supported by one of the 17 categories.

Here is the complete list for each category:
DetectorSerializationTypes.txt

Spike issue #112 Reconfigure the detectors to use global code for the Binary Serializers has a feature branch with a POC for FluenceOfRhoAndZDetector using global parameterized code. To support the serialization of all detectors during the transition, I added ReadDataGeneric in addition to ReadData, this would be changed back to just one Action ReadData once all detectors are implemented.

[dcuccia]

Thanks for thinking through this, @lmalenfant! I agree with your suggestion that we provide a default implementation, so that there is less duplicated code. And, appreciate the POC, as it helps us think concretely about the approach. Browsing the POC, I wonder if there's even more we can do here to simplify implementation. One concern I have is relying on static internal libraries - implementing "internal" detectors should "look just like" implementing detectors from a different assembly. I think we can swing that - I'll do a little work here and provide any feedback/suggestions. Thanks again - long overdue consolidation, on the path to making detectors as "small" as reasonably possible.

[dcuccia]

Made some more progress. Really like the small surface area - just one factory method, BinaryArraySerializerFactory.GetSerializer() - removes redundant implementation of iteration logic, but keeps the basic structure exposed for custom/differing implementations, right in the specific detector class that needs it:

public BinaryArraySerializer[] GetBinarySerializers()
{
    return TallySecondMoment
        ? new[]
        {
            BinaryArraySerializerFactory.GetSerializer(Mean, DetectorDimensionsInOrder, nameof(Mean), ""),
            BinaryArraySerializerFactory.GetSerializer(SecondMoment, DetectorDimensionsInOrder, nameof(SecondMoment), "_2"),
        }
        : new[]
        {
            BinaryArraySerializerFactory.GetSerializer(Mean, DetectorDimensionsInOrder, nameof(Mean), ""),
        };
}

Next I'd like to show/test usage in a more complex scenario, like ReflectedDynamicMTOfXAndYAndSubregionHistDetector. Please all let me know your thoughts! A possible addition could be to "hide" the default implementation (the code that decides whether to include the SecondMoment) by adding a helper extension method on IDetector<T>, and have an even simpler call, like:

public BinaryArraySerializer[] GetBinarySerializers() => this.GetDefaultSerializers();

Just wasn't sure if that was too much "magic". It wouldn't teach someone how they can manipulate the code to do something more custom/advanced. But the likelihood of someone a) wanting to do that, and b) unwilling to look one level deeper on how they could do that, is probably a vanishingly small number of people. The "GetDefault" name would hint that they could copy-paste that code and customize/tweak to their heart's content. Interested in feedback there - just wanted to start "smaller".

[dcuccia]

In fact, now that I read my own post, I realize that the detector dimensions interface addition is no longer needed. I'll update that.

[dcuccia]

I was able to remove all additional interfaces/members that I'd started with, so now everything to do with serialization is local to the function.

public BinaryArraySerializer[] GetBinarySerializers()
{
    var detectorDimensionsInOrder = new[] { Rho.Count, Z.Count };
    return TallySecondMoment
        ? new[]
        {
            BinaryArraySerializerFactory.GetSerializer(Mean, detectorDimensionsInOrder, nameof(Mean), ""),
            BinaryArraySerializerFactory.GetSerializer(SecondMoment, detectorDimensionsInOrder, nameof(SecondMoment), "_2"),
        }
        : new[]
        {
            BinaryArraySerializerFactory.GetSerializer(Mean, detectorDimensionsInOrder, nameof(Mean), ""),
        };
}

[dcuccia]

Still interested in your feedback @hayakawa16 @lmalenfant , but got a chance to implement the complicated detector. 283 lines of code collapses to the following:

public BinaryArraySerializer[] GetBinarySerializers() => EnumerateAllSerializers().ToArray();

private IEnumerable<BinaryArraySerializer> EnumerateAllSerializers()
{
    var meanDims = new[] { X.Count, Y.Count, MTBins.Count };
    var fractionalMTDims = new[] { X.Count, Y.Count, MTBins.Count, FractionalMTBins.Count };
    var mtOfZDims = new[] { X.Count, Y.Count, Z.Count };
    var subregionDims = new[] { NumSubregions, 2 };
    yield return BinaryArraySerializerFactory.GetSerializer(Mean, meanDims, "Mean", "");
    yield return BinaryArraySerializerFactory.GetSerializer(FractionalMT, fractionalMTDims, "FractionalMT", "_FractionalMT");
    yield return BinaryArraySerializerFactory.GetSerializer(TotalMTOfZ, mtOfZDims, "TotalMTOfZ", "_TotalMTOfZ");
    yield return BinaryArraySerializerFactory.GetSerializer(DynamicMTOfZ, mtOfZDims, "DynamicMTOfZ", "_DynamicMTOfZ");
    yield return BinaryArraySerializerFactory.GetSerializer(SubregionCollisions, subregionDims, "SubregionCollisions", "_SubregionCollisions");
    if (!TallySecondMoment)
        yield break;
    yield return BinaryArraySerializerFactory.GetSerializer(SecondMoment, meanDims, "SecondMoment", "_2");
    yield return BinaryArraySerializerFactory.GetSerializer(TotalMTOfZSecondMoment, mtOfZDims, "TotalMTOfZSecondMoment", "_TotalMTOfZ_2");
    yield return BinaryArraySerializerFactory.GetSerializer(DynamicMTOfZSecondMoment, mtOfZDims, "DynamicMTOfZSecondMoment", "_DynamicMTOfZ_2");
}

Here's the simple case:

public BinaryArraySerializer[] GetBinarySerializers() => EnumerateAllSerializers().ToArray();

private IEnumerable<BinaryArraySerializer> EnumerateAllSerializers()
{
    var detectorDimensions = new[] { Rho.Count, Z.Count };
    yield return BinaryArraySerializerFactory.GetSerializer(Mean, detectorDimensions, "Mean", "");
    if (!TallySecondMoment)
        yield break;
    yield return BinaryArraySerializerFactory.GetSerializer(SecondMoment, detectorDimensions, "SecondMoment", "");
}

I also expanded BinaryArraySerializerFactory to support 1D to 6D multidimensional arrays out of the box. Easy to add more, but this is already a couple more dimensions than we have in our current detector set.

What I like about this is the "locality" of the code - the new author doesn't have to look beyond the borders of the detector (as long as the size/shape of the underlying array is already supported. Excellent cycomatic complexity, and no hard dependency on a base class or other bits that are owned by the core library (out of the control of the new detector author (think, someone not recompiling Vts.dll, but just using it as a NuGet package - a much more common case).

This seem like a design we can move forward with? Happy to be the feature implementer on a clean branch. Could use help with adding some test coverage.

[lmalenfant]

I have been out for the past several days but once I have caught up with other work, I will pull the latest code and take a look.

[hayakawa16]

I am under the weather. I'll try to review when I'm feeling better.

[dcuccia]

Hope you feel better!

[lmalenfant]

I pulled down the latest code and there are 2 failing unit tests, I had this exact issue, when you are reading the binary, it reads the values from the text file and the Mean is null so you do actually need the value to be maintained in DataArray. This is the reason why I modified the BinaryArraySerializer to accept the IBinaryArray so I could access this value.

This data is not available in the detector so was tricky to resolve. I would check the code coverage for the other detector because it might not be covered by tests to show failures there also.

[lmalenfant]

It's hard to comment on the architecture because this will not work as is, the read is not getting the value from the binary. As I mentioned above, Mean is null.

I do like that you abstract away the array size but I'm not sure if that will work because the serialization doesn't just depend on the array dimensions. I mentioned at the beginning of this discussion that there are 17 categories of detector serialization but in reality probably only 13 different cases. If you look at those definitions some of the dimensions are Count - 1 but some of them use Count. We also need to accommodate Complex arrays.

[dcuccia]

Good news - I fixed that bug. Changes updated. The way this is working out, the detector owner is responsible for defining the appropriate size of the array (Count or Count -1), and they know which it should be (e.g. frequencies are Count, spatial/temporal bins are Count -1, fractional bins are Count + 1). Here's what the detectors look like now:

// FluenceOfRhoAndZDetector
private IEnumerable<BinaryArraySerializer> EnumerateAllSerializers()
{
    // Initialize all arrays to be serialized/deserialized before getting their serializer, in case they haven't yet been initialized
    // Todo: make independent variable properties init-only, so we can move this to the property initializer (no current guarantee that X, Y, Z are non-null)
    Mean ??= new double[Rho.Count - 1, Z.Count - 1];

    // return each array serializer with appropriate name and file tag info
    yield return BinaryArraySerializerFactory.GetSerializer(Mean, "Mean", "");

    if (!TallySecondMoment)
        yield break;

    // if tallying second moments, do the same for those
    SecondMoment ??= new double[Rho.Count - 1, Z.Count - 1];
    yield return BinaryArraySerializerFactory.GetSerializer(SecondMoment, "SecondMoment", "");
}

//ReflectedDynamicMTOfXAndYAndSubregionHistDetector
private IEnumerable<BinaryArraySerializer> EnumerateAllSerializers()
{
    // Initialize all arrays to be serialized/deserialized before getting their serializer, in case they haven't yet been initialized
    // Todo: make independent variable properties init-only, so we can move this to the property initializer (no current guarantee that X, Y, Z are non-null)
    Mean ??= new double[X.Count - 1, Y.Count - 1, MTBins.Count - 1];
    FractionalMT ??= new double[X.Count - 1, Y.Count - 1, MTBins.Count - 1, FractionalMTBins.Count + 1];
    TotalMTOfZ ??= new double[X.Count - 1, Y.Count - 1, Z.Count - 1];
    DynamicMTOfZ ??= new double[X.Count - 1, Y.Count - 1, Z.Count - 1];
    SubregionCollisions ??= new double[NumSubregions, 2];

    // return each array serializer with appropriate name and file tag info
    yield return BinaryArraySerializerFactory.GetSerializer(Mean, "Mean", "");
    yield return BinaryArraySerializerFactory.GetSerializer(FractionalMT, "FractionalMT", "_FractionalMT");
    yield return BinaryArraySerializerFactory.GetSerializer(TotalMTOfZ, "TotalMTOfZ", "_TotalMTOfZ");
    yield return BinaryArraySerializerFactory.GetSerializer(DynamicMTOfZ, "DynamicMTOfZ", "_DynamicMTOfZ");
    yield return BinaryArraySerializerFactory.GetSerializer(SubregionCollisions, "SubregionCollisions", "_SubregionCollisions");
    if (!TallySecondMoment)
        yield break;

    // if tallying second moments, do the same steps for those

    SecondMoment ??= new double[X.Count - 1, Y.Count - 1, MTBins.Count - 1];
    TotalMTOfZSecondMoment ??= new double[X.Count - 1, Y.Count - 1, Z.Count - 1];
    DynamicMTOfZSecondMoment ??= new double[X.Count - 1, Y.Count - 1, Z.Count - 1];

    yield return BinaryArraySerializerFactory.GetSerializer(SecondMoment, "SecondMoment", "_2");
    yield return BinaryArraySerializerFactory.GetSerializer(TotalMTOfZSecondMoment, "TotalMTOfZSecondMoment", "_TotalMTOfZ_2");
    yield return BinaryArraySerializerFactory.GetSerializer(DynamicMTOfZSecondMoment, "DynamicMTOfZSecondMoment", "_DynamicMTOfZ_2");
}

Will work on Complex next!

[dcuccia]

Implemented parallel support for Complex arrays. Once we can adopt .Net 7 or higher (8 is LTS and out in a month), we can collapse all the various type (Complex, double, etc) overloads into a single generic function. Here's ROfFx, for example:

public BinaryArraySerializer[] GetBinarySerializers() => EnumerateAllSerializers().ToArray();

private IEnumerable<BinaryArraySerializer> EnumerateAllSerializers()
{
    // Initialize all arrays to be serialized/deserialized before getting their serializer, in case they haven't yet been initialized
    // Todo: make independent variable properties init-only, so we can move this to the property initializer (no current guarantee that X, Y, Z are non-null)
    Mean ??= new Complex[Fx.Count];

    // return each array serializer with appropriate name and file tag info
    yield return BinaryArraySerializerFactory.GetSerializer(Mean, "Mean", "");

    if (!TallySecondMoment)
        yield break;

    // if tallying second moments, do the same for those
    SecondMoment ??= new Complex[Fx.Count];
    yield return BinaryArraySerializerFactory.GetSerializer(SecondMoment, "SecondMoment", "_2");
}

[lmalenfant]

I have some comments on the architecture but I want to make those later to give @hayakawa16 a chance to review. I will say that overall I like the architecture, I like that the serialization didn't need to change.

[dcuccia]

Cool, thanks!

[hayakawa16]

Hi, I have pulled the latest on the branch and stepped through the code. Looks good! I have a few comments/questions:

1. I remember you explained to me why using "yield return" was good for all of the ForwardSolver for loops. I kind of remembering it had to do with optimization (not sure if time or memory). Can you remind me? And is the same reason being applied here?
2. I see you have implemented updates to ReflectedDynamicMTOfXAndYAndSubregionHistDetector. Your changes make sense to me. Thanks for implementing this type of detector that outputs more than Mean and SecondMoment so I can see how they would be coded.
3. There is a "Todo" in your changes: "make independent variable properties init-only, so we can more this to the property intializer (no current guarantee that X,Y,Z are non-null)". Do you plan to implement this todo?
   Sorry for my delayed response. Still on the mend but wanted to respond.

[lmalenfant]

@dcuccia I wasn't a fan of all the yield returns so I do actually like solution 3, it would be good to see that version with the other detector though because maybe that one will be less readable. I still defer to @hayakawa16 for the final decision though because I don't actually use this code.

My other comment I wanted to make was that I think we can refactor the code in BinaryArraySerializerFactory:
https://github.com/VirtualPhotonics/VTS/compare/feature/112-reconfigure-the-detectors-to-use-global-code-for-the-binary-serializers#diff-54bb197a54f17f3bac1a6a44e05bfd6fd59e47cd53ab574e955ed2fbf740228d

There is a lot of repetition with these methods and I had already worked with similar code (in IEnumerableArrayExtensions) to create a method that could iterate though any dimensional array. I wanted to run extensive benchmarking before adopting this code so it is currently only used in unit tests.

VTS/src/Vts/Extensions/IEnumerableArrayExtensions.cs

Line 210 in cc75823

internal static IEnumerable<T> ToEnumerable2<T>(this Array myArray) where T : struct

We could also leave this code as is and try and refactor later, I just wanted to mention it because I had worked on this recently.

I think we should discuss the shared code solutions in a separate discussion, I think making incremental changes like this one and then improving on them later is the best way forward.

[dcuccia]

Thanks for the feedback, @lmalenfant :).

I wasn't a fan of all the yield returns so I do actually like solution 3

Out of curiosity, what don't you like (and/or why does solution 3 appeal to you)? Any feedback about the following rationale?

I'd worried that maybe it wasn't as easy to understand/duplicate (and it leaves something possibly null - the implementer must remember to filter for non-null cases at the end.

It should be similarly straightforward for the more complex case.

Re: duplication - thanks for your thoughts and work, and I agree. I was thinking we could use the iterators available on the Span<T> exposed with multi-dimensional helpers like those in the in the .NET Community Toolkit High Performance library (docs, github) to iterate over all the different cases, for performance's sake. They have 2D iterators, but it should be straightforward to extend to higher orders. But since iterating MD arrays is such a cross-cutting thing in our solution, I'd thought to set it aside.

If you'd like to tackle now/here that sounds great. Like you say, performance matters...not necessarily for saving/loading results as much as other places, but still. I've been doing a lot of comparative performance testing with BenchmarkDotNet on this very thing, and for accelerating with SIMD operations where available. It perhaps could be it's own "small" investigative feature branch, picking one aspect like this serialization as a trial run?

[dcuccia]

Misspoke, extensions currently exist for 1, 2, and 3D, and you can see how we could easily add higher order versions with the same structure: https://github.com/CommunityToolkit/dotnet/tree/ecd1711b740f4f88d2bb943ce292ae4fc90df1bc/src/CommunityToolkit.HighPerformance/Extensions

[lmalenfant]

Out of curiosity, what don't you like (and/or why does solution 3 appeal to you)? Any feedback about the following rationale?

Yes, solution 3 is a more compact solution which I like. The reason I didn't like the yield returns as much is because this would need to be implemented by the developer coding the Detector, I find the yield returns to be less obvious and think there might be more chance of them doing it wrong.

@dcuccia Could you look at the code in IEnumerableArrayExtensions and let me know what you think? This code has the potential to iterate through arrays up to 32-dimensions (I believe this is the .NET limit).

From first glance, the GitHub code that you provided the link for still appears to be specific to the dimensions.

[dcuccia]

@dcuccia Could you look at the code in IEnumerableArrayExtensions and let me know what you think?

At the core of it, GetValue() returns an object, so it causes boxing/unboxing to iterate a struct-backed array. If we are ok with this for specific areas that don't need higher performance, I'd recommend the following 1-liner:

internal static IEnumerable<T> ToEnumerable3<T>(this Array myArray) => myArray.Cast<T>();

We can leave the Span stuff for some other time, and do it when we are looking specifically for performance gains. Lots there I'd love to work on if/when we collectively have time.

[hayakawa16]

Thanks for your responses! You mentioned: The last version 3 is the shortest and the first thing I wrote, but I'd worried that maybe it wasn't as easy to understand/duplicate (and it leaves something possibly null - the implementer must remember to filter for non-null cases at the end.
Was version 3 committed? I was wondering if I could pull that version and try it.

[dcuccia]

No problem. I didn't commit the code, but you should be able to copy-paste the above and replace/rename the yield return version to test. Could be an interesting test to see if you can easily duplicate on a different detector. The one watch-out is to make sure you use N, N-1, or N+1 array sizes, depending on the type of independent axis (e.g. endpoints or frequency "midpoints", etc).

[hayakawa16]

Would it be okay if I tried implementing it on another detector on your branch?

[dcuccia]

Please do! Lisa started the branch, actually.

[hayakawa16]

I implemented Solution 3 in two new detectors: AOfRhoAndZDetector and ReflectedDynamicMTOfRhoAndSubregionHistDetector. It was very straight-forward to implement and all unit tests pass. I did the second detector because it serializes additional arrays. On the 2nd moment for the additional arrays, I used the "TallySecondMoment ? " preface to all. This could possibly be simplified.

[dcuccia]

Super. :) For each is probably the simplest for now, avoids the procedural code of solution 2.

[hayakawa16]

Once we finalize an implementation, I'd like to volunteer to update the rest of the detectors, since I use the code the most.

[dcuccia]

Thanks! @lmalenfant would you like us to create a new branch, or reorganize this one?

[lmalenfant]

I think once we start working on this, we need to create a new branch because there is a lot of discarded code in this one.

@dcuccia I mentioned in a previous comment about finding a solution for BinaryArraySerializerFactory that wasn't so repetitive, can we explore that before we move forward with implementation because if we implement this version, I'm not sure it will get refactored later?

[dcuccia]

@lmalenfant my opinion is that the current implementation is straightforward and easily extensible "write once and leave it alone" code that provides decent performance and won't need to be changed by the general user, but as I mentioned in the other thread, if we want to replace with myArray.Cast<double>() and live with the boxing performance loss that's fine too. Duplication's not inherently bad if it works and is modified almost never (just as our original ToEnumerable() code was), but I defer to the team's decision.

[lmalenfant]

We don't have to write it that same way, I am always looking for a more elegant solution, it's not just about just avoiding the repetition either, the ability to support any number of dimensions without changing the code appeals to me. This is possibly not something we need but I just lean towards more compact code, of course if there is performance degradation we can just keep the original code.

[dcuccia]

Yeah, spent a little time on the Span/Memory options that could be the best of both worlds, but I think continued reliance on multi-dimensional arrays means we're limited without some more boilerplate support (e.g. from the .Net Community Toolkit, but that's only 1D-3D). I think a re-imagining of how we do indexing, in light of Span.Slice() capabilities, could result in major optimizations, but those are probably best undertaken with data-driven focus on the hot spots in the code, and serialization probably isn't one.

[hayakawa16]

I modified Method3 and pulled the array assignment out of the expression. Code now looks like:
public BinaryArraySerializer[] GetBinarySerializers()
{
Mean ??= new double[Rho.Count - 1, Z.Count - 1];
SecondMoment ??= new double[Rho.Count - 1, Z.Count - 1];
var allSerializers = new List
{
BinaryArraySerializerFactory.GetSerializer(
Mean, "Mean", ""),
TallySecondMoment ? BinaryArraySerializerFactory.GetSerializer(
SecondMoment, "SecondMoment", "_2") : null
};
return allSerializers.Where(s => s is not null).ToArray();
}

If this looks okay, I plan to implement in all detectors.

[dcuccia]

Sounds good!

[lmalenfant]

This work will continue in issue #112