Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

72 Implement and test script-style use of the VTS #86

Conversation

dcuccia
Copy link
Contributor

@dcuccia dcuccia commented Aug 16, 2023

Initial translated samples that demo "script-style" use the Vts, like in the Matlab scripts, as a foundation for consumption in .Net Polyglot and Jupyter Notebooks. For now, all 12 Monte Carlo demos are implemented and 11/18 of the Forward Solver demos are implemented.

Commented here to see how much more we ultimately want to do. IMO those could be added now, or at some time in the future. It is highly likely that these will need to evolve anyway once we implement the notebooks, as we'll want to keep some level of parity on how the data is being sliced/diced and plotted. So, I suggest we consider having additional changes to these in the core library tracked in a separately Item, such as "Enhance and align script demos to those used in notebooks."

Note: It's unclear how/when we should include the Vts.Scripting.Tests in a CI process - it should probably be part of an integration test step, but we don't have that layer of granularity (yet - it's something I'd like to help with). Like the Matlab scripts they currently open up new windows that need to be closed. For now, just knowing that these libraries compile, and that the script tests pass when run ad-hoc is ok with me.

dcuccia and others added 30 commits July 27, 2023 14:12
…TS fix to ensure directory before creating database files, rename top-level programs for clarity.
…nstead of GetPhotonDatabase to set up for PhotonDatabasePostProcessor overload call.
…nd align Matlab and C# usage of final bin for inversion (do not include last bin) in MC07
@hayakawa16
Copy link
Member

Regarding forward solver demos: thanks for addressing all of my comments! Just a few responses.

  1. I did not realize that Demo04FluenceOfRhoAndZAndFt put out 3 plots. Its hard to know what tabs are new when running in succession. Now that I see the 3 plots, I see why the third is titled "Modulation". Does everyone know that it is the ratio of the prior 2 plots?
  2. glad my comments helped to debug :)
  3. I'm going to see if I can debug the line issue with Demo07PhdOfRhoAndZTwoLayer and Demo06FluenceOfRhoAndZTwoLayer
  4. Steve Jacques chapter 5 in the purple book says: Total energy absorbed [J/cm3] can be computed when the energy [J] of the light source is given. When talking about Monte Carlo solutions, he says: It is convenient to consider the Monte Carlo simulation as yielding the fractional density matrix of incident light absorbed, A [1/cm3], in response to one unit of delivered power or
    energy. I think all of our models assume a normalized source.
  5. Demo10ROfRhoAndTimeMulti looks good.

Regarding ShortCourse folder: thanks for all of the fixes, they look great!
Analog is indeed a binary estimator and so no weight modification is performed to the photon due to absorption. That is why I didn't like the name "AW" (absorption weighting) used with this random walk process. DAW and CAW random walk processes disallow termination of the photon due to absorption and instead deweight the photon along its trajectory and so "AW" in their name makes sense.

Thanks for your quick turn around!

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

Thanks.

Does everyone know that it is the ratio of the prior 2 plots?

Anyone reading the script will know. In the future I'd like to combine back in one, but suggest we first figure out how to resolve the colorbars issue.

fractional density matrix of incident light absorbed, A [1/cm3], in response to one unit of delivered power or
energy.

So it's normalized for use with either power and energy, as we hypothesized, thanks!

Re: analog, I guess I was just saying it is binary absorption weighing: 0 if not terminated, 1 if so. But all fine w/ me re: naming!

….x999999999999999. This removes light line that was seen at about z=0.7mm.
@hayakawa16
Copy link
Member

On Demo06FluenceOfRhoAndZTwoLayer if I set the number of rhos and zs to 50 instead of 100 the line is not there. It also isn't there if I set the "stop" for the rhos and zs to be 9.9 instead of 19.9. So it isn't a fineness of grid thing. Here is my guess, it is a floating point issue. If the rhos and zs are set to start:0.1, stop:19.1, number:100, then the rhos and zs values are: 0.1, 0.3, 0.5, 0.7, 0.899999999999991, 1.0999999999999999. This is right where problem is. If I set the rhos and zs to start:0.1, stop:20.0, number:100, then there is no line because no rhos and zs values have form x.x999999999999999.
Do you buy this? I have pushed these fixes to Demo06 and Demo07 of ForwardSolvers so you can take a look.

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

@hayakawa16 thanks for looking into this. I appreciate the thought, but I'm not sure I buy it. This integer computation gets much closer to the double-precision limit of accuracy, but also produces the same line:

        var rhos = Enumerable.Range(1, 100).Select(i => ((double)(i * 2 - 1)) /10).ToArray(); // range of s-d separations in mm
        var zs = Enumerable.Range(1, 100).Select(i => ((double)(i * 2 - 1)) / 10).ToArray(); // range of depths in mm

(as does this, but it has the same double precision issue):

        var rhos = MathNet.Numerics.Generate.LinearSpaced(100, 0.1, 19.9); // range of s-d separations in mm
        var zs = MathNet.Numerics.Generate.LinearSpaced(100, 0.1, 19.9); // range of depths in mm

And if you zoom in, the data are "real" (and changing along rho!), but an order of magnitude larger than the rest:
image

@hayakawa16
Copy link
Member

I have another idea. This is an SDA solution. One of the key assumptions about when you can apply an SDA solution is if mus'>>mua. The top layer OPs were set to mua=1, mus'=1. This violates this assumption. If you set the top layer OPs to mua=0.1, mus'=1, and bottom mua=0.01, mus'=1, then line is not there I think. What do you think about that?

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

Did a little more experimentation - it seems to only show up for me when the top layer optical properties are mua: 1, musp: 1. Change them slightly, and it will go away. Changing bottom layer properties doesn't matter, it seems.

…. Modified OPs to ones that abide by SDA assumption that mus'>>mua.
@hayakawa16
Copy link
Member

I reverted my rhos and zs modifications, and modified OPs so that line is not there. I pushed this fix.

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

Ok. Is it worth documenting the bug as a separate ticket? Some singularity with the TwoLayerSDAForwardSolver.StationaryFluence function?

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

And are we otherwise ready to approve/merge?

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 21, 2023

Missed the comment you made earlier, sorry. Yes I agree, if we see it pop up again for some other SDA-congruent optical properties, then maybe it's worth tracking as a bug.

@hayakawa16
Copy link
Member

On the first comment, at first I thought we violated the assumption that the top layer needs to be greater than l* because the embedded source at l* deep needs to be in the top layer. This is well documented in the header of this class and there is code to check for that assumption. I'm not sure we can know a definite lower bound on mus'/mua when the solution becomes invalid, however I guess adding a check if mus'==mua (and I think this check should be done for both layers) could be done and adding documentation to the header in this regard. I can create an issue for that if that is what you had in mind.

@hayakawa16
Copy link
Member

This check should be added to all SDA forward solvers if they don't already have it.

@hayakawa16
Copy link
Member

Issue generated and assigned to me. I noticed that the check I made to ensure top layer thickness is thick enough is in each TwoLayerSDAForwardSolver method. Might be a better way to do this going forward. I might ask advice from those with stronger coding skills than mine as to best way to implement this check.

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 22, 2023

Thanks Carole! FWIW, I don't think it's related to musp==mua or limits. The original settings of (1,1) were the only thing I could get to generate the problem, and again only for the top layer.

@hayakawa16
Copy link
Member

I think the check should be in the SDA forward solvers. Just because we don't see a visual aberration, doesn't mean the solution is valid.

@lmalenfant
Copy link
Member

And are we otherwise ready to approve/merge?

Once you and Carole are good with the changes, I will take one last pass and we should be good to merge!

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 22, 2023

I think any other changes would be in a different branch. I think we're good here. Carole?

@hayakawa16
Copy link
Member

My technical review is complete. I'll let Lisa approve after her final pass.

@lmalenfant
Copy link
Member

lmalenfant commented Aug 22, 2023

I am unable to build the project, I think due to the static abstract in the IDemoScript interface.

This is the error:
Using both 'static' and 'abstract' modifiers requires opting into preview features.

@lmalenfant
Copy link
Member

@dcuccia can you look into this and then we should be ready to merge:

I am unable to build the project, I think due to the static abstract in the IDemoScript interface.

This is the error:
Using both 'static' and 'abstract' modifiers requires opting into preview features.

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 22, 2023

Not sure what's going on here, as the preview features are enabled, per the .csproj file. I thought it might just be your machine, but right-clicking on the project (and running "dotnet build") and have the same problem. But, if I click the green "Play" button with the Vts.Scripting project as the active project, it works - and, I can see the Debug assets are built with a "just now" timestamp. Can you let me know if the same is true for you?

@dcuccia
Copy link
Contributor Author

dcuccia commented Aug 22, 2023

That said, I pushed a change that removes the contract for now, until we figure out what's going on. It's not essential to the use of this code, going forward, just a good guardrail for making new examples uniform (which will happen by copy-paste even without this).

Copy link
Member

@lmalenfant lmalenfant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew! We made it!

Not sure what's going on here, as the preview features are enabled, per the .csproj file. I thought it might just be your machine, but right-clicking on the project (and running "dotnet build") and have the same problem. But, if I click the green "Play" button with the Vts.Scripting project as the active project, it works - and, I can see the Debug assets are built with a "just now" timestamp. Can you let me know if the same is true for you?

Yes, that was the same for me.

@dcuccia dcuccia merged commit f96eec1 into master Aug 22, 2023
@dcuccia dcuccia deleted the 72-net-polyglot-notebook-and-jupyter-notebook-documentation-and-samples branch August 22, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement and test script-style use of the VTS
3 participants