VirtualPhotonics / VTS

Virtual Tissue Simulator
https://virtualphotonics.org
Other
34 stars 9 forks source link

RangeOfT enhancements for simplicity to consumers #90

Closed dcuccia closed 1 year ago

dcuccia commented 1 year ago

This was discussed in https://github.com/VirtualPhotonics/VTS/discussions/81 and is rearing it's head as I try to make the Notbooks simple and refactor commonly used code/extensions to the Vts core.

Originally posted by **dcuccia** August 11, 2023 I've found, in creating the user samples, the utility of having extension method operators on `Range` objects. The need to call `myRange.AsEnumerable().ToArray()` is unconcise and not very intuitive to newcomers. It will also be that much more confusing if we need to add any additional calls to make it compatible with PythonNet. In order to resolve these, I'd propose: 1. at a minimum, add a `ToArray()` extension method in a new `RangeExtensions` class 2. consider instead making the range itself implement IEnumerable, so that someone can benefit from all the LINQ goodness directly. E.g., they could say: ```csharp double[] endpoints = myRange.ToArray(); List endpoints = myRange.ToList(); foreach(double endpoint in myRange) { //... } ``` This would also clean up the code significantly throughout the codebase. Only check would be to ensure that any WPF databinding to a Range isn't disrupted by it newly implenting `IEnumerable` (sometimes there are weird defaults, but this one is easy to check that databinding continues to work).
lmalenfant commented 1 year ago

I put an upvote on this discussion, I probably should have made a comment also. If you would like to create a separate branch from master and make this change, we will be able to pull the Vts.Gui.Wpf into the solution to make sure the binding still works as expected.

dcuccia commented 1 year ago

Cool, thanks Lisa! I'm on it. :)

lmalenfant commented 1 year ago

Cool, thanks Lisa! I'm on it. :)

Thank you!!

lmalenfant commented 1 year ago

There are multiple failing unit tests in the VTS, MCCL and PP (192), is it possible that this has affected the serialization/deserialization. When we make changes like this to the library we have to run unit tests before we create the PR. I think this code change needs investigating more.

dcuccia commented 1 year ago

Ok, I'll take a look. Pretty sure this is a JSON.Net issue that I've fixed before. Sorry - was trying to get something out to you quickly yesterday.

dcuccia commented 1 year ago

Pushed the small fix (this one gets me every time). Sorry for the churn - I'm used to pushing my changes to my remote branch and getting build/test automatically done for me, which blocks PR creation in the first place.

dcuccia commented 1 year ago

(all tests passing now)

lmalenfant commented 1 year ago

Thanks! Let me review it on the branch first before recreating the PR.