-
Notifications
You must be signed in to change notification settings - Fork 9
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
Implement and test script-style use of the VTS #72
Comments
To follow naming convention of the other projects, the test application should be named Vts.Scripting.Test and it needs to be created now alongside this project. It is much easier to write unit tests at the same time as the code being tested. As per our SonarCloud analysis, at least 80% code coverage is what we require for new code. |
Sounds good, thanks @lmalenfant ! |
@hayakawa16 I wonder if I could enlist your help. I just flushed out a pMC scripting demo (corresponding to Demo 6 in the Matlab Monte Carlo demos), and for some reason, all three OPs appear to be post-processed to the same result. This is the file: Would you mind doing a sanity check for me, and maybe comparing with Matlab? (I don't have Matlab anymore...too expensive). You can check it out under branch #72, and be sure to check that when you open MonteCarlo-06-pMCPostProcessor in Visual Studio, the built-in nuget tool SmallSharp changes the "launchSettings" to specify that file (a work-around to have multiple top-level programs in the same .csproj). If not, you can manually change it in the drop down. It should look like this, and then you can click the solid "play" button (on the left) to run that script: |
Hi dcuccia, First, I ran Example 6 in vts_mc_demo.m in the MATLAB interop download. This is the plot I get: First question: are you plotting on a log scale because with only 1000 photons being run, the changes might be hard to see on regular scale. Maybe this solves your question. If not, I will try to checkout your branch and see what happens. |
Thanks Carole. I see - I didn't actually check to see if they were identical. Will do that and also run more photons and report back. (Yes, though, that was a log plot). |
Let me know how it goes. I can try to pull your branch later this week. |
Following the "number 6" matlab example I simulated mua = 0.01, musp = 1.0 for the baseline, and then updated the perturbation numbers to mua = 0.5/mm and 2.0/mm for the "half" and "double" cases (just to be extreme), and I still see fully overlapping plots: Maybe if you have time @hayakawa16 you could run this as well to spot any issues with the logic? Probably a bug in how I've set up the pMC, but I don't "see" it (probably too close!). |
Hi, I checked out your branch and opened it visual studio. I set Vts.Scripting to the startup project. Then clicked on MonteCarlo-060pMCPostProcessor.cs. I see this title in the run window like your image. I click the green right arrow but get an exception: The database file could not be found. I think I'm doing something wrong. |
One thing I see just visually. On line 41, the database set is pMCDiffuseTransmittance. In Example 6, the database is set to pMCDiffuseReflectance. I see that the slab is 100mm thick and this may be the problem. |
Ah, I'm sorry - I did fix that bug locally but hadn't pushed yet. These are created with DiffuseReflectance as the virtual boundary. |
I can change on my side. Ah, now I'm getting a plot! Okay I can try to debug. |
I figured it out. Do you want me to push my changes or would you rather I describe here what I did. |
Yes, please go ahead!
|
Committed and pushed. |
Just as an aside, I made the fix but didn't realized I didn't have your commit on Aug 4, so I pulled and then pushed my fix. Now in VS I can't seem to get MonteCarlo-06-pMCPostProcesor to show in run window. So I wasn't able to run it after the pull. |
@hayakawa16 thanks - I see that I had the wrong type of photon database (maybe we should have better compile-time constraints). Anyway, that all worked out great, and on to the next thing... No rush on this, but when you can, please check out and run my example 07 and let me know what you think. I realized that we didn't have a pMCForwardSolver, so I made one (for the demo only, at this point). I still need to work on the plot formatting, but the Lev-Marq optimizer seems to do it's job.... |
That result look good. I see though that you are including all rho bins and remember that the final rho bin gets everything reflected beyond that bin added to it (so that total diffuse reflectance can be checked and validated). So when I run inversions, I define my rho bins so that I can exclude the final bin in the fitting. Surprisingly your result seems to accomodate that! But in general best not to include final bin. |
I just ran the vts_mc_demo.m Ex. 7 and it appears we run with that last rho bin included too! |
Excellent, thank you. And good point - thanks for the reminder on that last bin. Perhaps we should update all to remove the last bin. In the meantime, I've added Example 08, and noticed that the "unit test" criteria are strangely, barely not passing. The issue is the middle (complex) number for ROfRhoAndOmega. |
Update: I've added MC09, so that's all of the MC demos with the two caveats above (final bin issue/alignment in MC07, and accuracy check in MC08). Some plot formatting would be nice too. |
I see problem. The var omegaRange needs to start at 0.05 (not 0..5). |
I pushed fix. |
Oops! Thank you! |
Nice! I see you're using Plotly. Just curious, what are advantages of this package over other python visualization libraries? |
Plotly is pretty common in Python science and engineering work, which is why I thought it would be a good common denominator between the C# examples and Python ones (via pythonnet). It's built on JavaScript and is the backbone of tools like Dash, so it's got a lot of support/community (to the level that there's now Plotly.net that wraps it for F# and C#). Given, I don't have a ton of experience with it yet, so it will take a little digging to get the plot formatting just right. FWIW, per the above plot in FS03 I took some liberty in the C# to plot the heat map of internal fluence(rho, z) at 1000nm. The corresponding example in Matlab instead plots fluence(rho=0, z) vs wavelength. Thoughts on which is a better "slice" to show at the end of the computation? |
Hard to know what would be best slice. I think having comments about order of indices (column major) so that the user can dissect themselves is always good. I noticed from the code, that the plot above is for the last wavelength, however the plot doesn't list that wavelength. If it was shown then user would know which is the fixed variable and that the data used for the plot actually has that many independent variables. I'm not sure how flexible the plotting is but if "Phi(rho,z,820nm)" could be shown or something like that. |
That looks great! I like the "square" aspect ratio too. |
Thanks @hayakawa16! Two next questions for you:
|
Hi, to answer your questions:
|
Thanks!
Ok, thinking ahead, maybe it would be worth it to curate/order the samples to teach the use and breadth of the Vts, and separate those from one-off demonstrations (which are super valuable, but don't need to be "supported" in the sense that they don't need to be continually tested, refactored, and synchronized with the matching notebooks). I think that's the beauty of the notebooks, which can be shared (and searched) in Q&A Discussion threads here in Github.
Awesome - I'll translate |
@dcuccia I was thinking about the next project with the Python scripts and I think we should do it the same way as this one in case you discover any more bugs in the VTS code. We can create the Vts.Scripting.Python (I'm open to other naming suggestions) project in the VTS repository and then once we are ready with the code we can either merge it to master and then move it out later or create the next VTS release then and move it out into a separate Repo referencing the VTS NuGet package. |
@lmalenfant I like the idea, and would hope we can still commit to getting them segregated to their own projects in the relatively near-term, because as you suggested earlier, the audience for the different languages could vary significantly. Practical question - do you want me to pull the PR and continue work here, or can we merge this one and create a continuation ticket (my preference)? Re: naming/organization, I suggest we re-organize the root 'Matlab' folder into a sub-folder called examples. Then, we can have all sorts of demos of use of the framework under that umbrella, including Scripting/Matlab, Scripting/Python, Scripting/C# (Polyglot Notebooks), maybe even Scripting/JavaScript. :) |
Yes, as soon as it is complete (or as complete as it needs to be to be useful) we will move it to its own repo. We can release to NuGet any time going forward.
The next phase should have its own issue and branch, let’s get your first PR reviewed separately. Nothing is preventing you from either branching from master or your branch (if you need code from there) and starting work on the next piece, while we review your PR. It might take me a few days.
Message ID: ***@***.***>
|
Sounds good, thanks! |
I made some notes on the PR itself, but just to follow up to the above, this is now ready for review. |
Discussed in #70
Main Idea
In order to lay the groundwork for future Jupyter Notebook and .NET Polyglot Notebook sample code, as envisioned in #70, this ticket is to create sample C# "scripts" (top-level statements) that mimic the existing VTS Matlab scripts. In addition to unblocking the future work, at the end of this, anyone more comfortable working out of the VS or VS Code IDE should be able to use these scripts directly for their work if they desire.
Details
Design details:
Vts
solution, with project name & namespaceVts.Scripting
. This will ensure building the solution catches any build-level regressions to the scripts that might occur when the VTS library changes.Vts.csproj
project within theVts
solution, to facilitate full use of the tools for design-time compilation error checking and debugging. This will differ from the future "external" Notebooks, which will resolveVts.dll
from Nuget.Vts.Scripting.Test
project to cover the new script samples to catch/flag any runtime issues/bugs that will necessarily need updates to the libraries in the other reposAcceptance Criteria
The text was updated successfully, but these errors were encountered: