Fixing accessibility of mus in MC JSON input file about vts HOT 25 CLOSED

My solution to this issue is to remove the serialization of Musp to/from the JSON infile. This is achieved by changing [DataMember} attribute of Musp in the Optical Properties class to [IgnoreDataMember]. The nice thing about this fix is that the old infiles that still include Musp will still work with the new code. The serialization ignores the Musp and determines it from the Mus and G and the results obtained are identical to the original results.

from vts.

Actually, I should add a comment about identical results. The way the original code worked is that if the Mus and Musp were set in the infile in an inconsistent manner, i.e. Musp was not equal to Mus*(1-G), the the code used Musp and modified the Mus value. So if an old infile has the Mus and Musp set in an inconsistent manner, then the results using the new code will not be identical to those obtained with the old code.

from vts.

from vts.

Thank you for your feedback David. I recall our prior conversation about this years ago and I was hoping you might post.

i see your point in that a user of the MC might want to study a series of simulations in which mua and musp are held constant (and therefore lstar is constant) and g varies. Conversely, a user might also want to study a series of simulations in which lstar and g are held constant and mua and musp (or mus) vary. I have done many of the latter type studies. So I think it is a matter of preference.

The only concern I have is that typically Monte Carlo codes ask for mus not musp. I don't know of one example of code that uses musp instead of mus. All of these codes specify mus in their input: MCML, TIMOS, or MCX. I think it is in the nature and history of the solution.

Please let me know your feedback.

from vts.

from vts.

I appreciate your comments. Unlike all of the Monte Carlo codes that I listed, our code base includes diffusion solutions so we have more requirements on our code design. So if I make the Mus attribute [IgnoreDataMember] and keep Musp attribute [DataMember] so that Musp and not Mus is in the MC infile, is that the preferred solution? If so, let me ponder this. I'm softening but need to try it on longer ;)

from vts.

from vts.

What if we leave OpticalProperties class as it was, i.e. [DataMember] on all properties. Then write a method in FileIO that allows the SimulationInput method ToFile to exclude Musp from the serialization. Something like this:
https://blog.rsuter.com/advanced-newtonsoft-json-dynamically-rename-or-ignore-properties-without-changing-the-serialized-class/
Do you think something like that would work?

from vts.

from vts.

Thank you very much for your post. I understand better what you'd like to have happen, to have both triplets be readable, and to be backwards compatible allow both to be defined. I like that solution.

I read your post and the SO post several times and tried to code up the idea. I think I must have posted problems here about 5 times prior to this post ;)
I got this to work!!! I created an OpticalPropertiesConverter class per the SO post, added it to VtsJsonSerializer KnownConverters and then figured out logic to put in the OpticalPropertiesConverter ReadJson method to work if the infile has Mus but not Musp, Musp but not Mus, and both. I compared the resulting binary (not all, just one so far) with the prior release of the code and the binaries are identical!

The only catch I see is that the infile that gets written to the results folder has only Mus written. If I change [IgnoreDataMember] attribute of Musp back to [DataMember] the results infile has Mus and Musp listed and everything else works fine.

David, thank you for your help. I truly could not have done this without your concept and the example code.

from vts.

from vts.

Thanks for reminding me about the MC Matlab post-processing code. I have verified that the load_results_script.m runs fine with these changes.

The infile that is copied to the results folder serves two purposes: 1) as documentation as to what input created these results, and 2) as an infile to be able to generate the same results. Argument for writing both Mus and Musp: 1) and 2) the satisfied, and if only Mus is specified in the infile and Musp is written to the results infile (or vice versa), that could be confusing. Argument against writing both and just writing one: perpetuates specifying just one, and 1) and 2) are somewhat satisfied although there could be this confusion.
Leaning towards writing both, but could be swayed.

Not sure I understand your comment: Probably calling musp constructor for forward solvers is the best when comparing different models (diffusion, mc). The OpticalProperties class did not change at all so still takes in Musp as it always did and sets Mus under the hood. And the selection of Mus, Musp or both only plays a part upon deserializing the MC infile. I don't think any other code deserializes json? So I don't think there should be any change to any forward solver diffusion or mc (scaled basic and scaled nurbs).

from vts.

from vts.

I tried that at first but can't have two constructors with the same signature (double, double, double, double). So the way I coded it is that the OpticalPropertiesConverter, takes in either Mus, Musp or both (along with G and N0 and then determines the appropriate Musp to call OpticalProperties class. Did I misunderstand?

from vts.

from vts.

I see. Would you like me to push to a branch so you can see what I did? I've committed but haven't pushed yet.

from vts.

from vts.

Okay, I just pushed to branch ch-181217-deserializeOPs-options.

from vts.

Sorry wait a second. I messed something up.

from vts.

Okay I think its fine. I just cloned a clean copy just to make sure. The branch has a first commit that contains 3 changes: new OpticalPropertiesConverter.cs, Vts.Desktop.csproj change to add link there, and VtsJsonSerializer.cs. I did another commit but that has changes to test MC Matlab. I plan to remove second commit changes.

from vts.

Any opportunity to review this yet?

from vts.

I plan to merge this branch into the master soon.

from vts.

from vts.

Thanks for your response David!

from vts.

I added unit tests to validate the software. I created three infiles that have tissue definitions with 1) Mus defined only, 2) Musp defined only, and 3) Mus and Musp defined and inconsistent. For all three the unit tests verify that the code runs the infile. For the last inconsistent infile, the unit test verifies that Mus was updated to be consistent with Musp prior to running.

I have merged this branch into master.

from vts.