VirtualPhotonics / VTS

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

Update to RangeOfT to implement IEnumerable<T> directly and simplify usage in the codebase #91

Closed dcuccia closed 1 year ago

dcuccia commented 1 year ago

Updated RangeOfT to implement IEnumerable directly, and updated examples directly referenced by the source code using refactoring tools. Obsoleted (but preserved) the existing ToEnumerable() class method, which is now unnecessary. Consider removing the public interface to this method at some future point, once the API has published this Obsolete message for a release or two.

dcuccia commented 1 year ago

I think this does the trick, but if someone could confirm that unit tests pass and that the WPF GUI still binds properly to the range variables, etc, that would be lovely!

lmalenfant commented 1 year ago

I'll run the unit tests next week and pull in the WPF to validate the binding.

dcuccia commented 1 year ago

Thanks!!

dcuccia commented 1 year ago

I should note, I decided not to touch the Matlab files, as those are already getting some churn on the notebooks branch and this is a non-breaking edit. Easy enough to remove any calls to AsEnumerable() on the notebooks branch itself.

lmalenfant commented 1 year ago

@dcuccia you mentioned that this is a non-breaking edit but there was one line using AsEnumerable in the WPF that needed to be removed so it would compile. Can you check and make sure the MATLAB is still working? After I made the fix in WPF and ran the GUI, the ranges look be working as expected so we are good there.

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.

lmalenfant commented 1 year ago

Closing this PR because it is not ready for review, we can keep discussing on the issue.

dcuccia commented 1 year ago

I reopened, as it was a small change and everything's passing now.

dcuccia commented 1 year ago

I tried to maintain backwards compat in the change, but the broken WPF code helped remind me that the code was AsEnumerable(), not ToEnumerable(). Fixed that now.

lmalenfant commented 1 year ago

Did you check the MATLAB unit tests also?

dcuccia commented 1 year ago

No, I don't have Matlab on any of my machines

dcuccia commented 1 year ago

But hopefully the AsEnumerable() keeps them chugging along.

lmalenfant commented 1 year ago

@dcuccia you are good to pull in the updated master.

Let me know once you are finished and I will review again. I just need to pull in the WPF again and run all tests including MATLAB and we should be good.

dcuccia commented 1 year ago

@lmalenfant @hayakawa16 this should be good to go. All tests passing off of the latest main.

lmalenfant commented 1 year ago

@dcuccia I deleted your branch, hope that was ok. I usually do it immediately after the merge to avoid branch confusion.

dcuccia commented 1 year ago

No problem, thank you. Will try to remember in the future.