VirtualPhotonics / VTS

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

R(fx) tallies assume a homogeneous or layered tissue geometry #13

Closed dcuccia closed 1 month ago

dcuccia commented 6 years ago

Ran into this:

"R(fx) tallies assume a homogeneous or layered tissue geometry; Change tissue type to be homogeneous or layered"

Not true in the general case. Think this should be a warning, not an exception.

hayakawa16 commented 6 years ago

Hi dcuccia, I'm assuming you mean the SimulationInputValidation exception when a SingleEllipsoidTissue is specified and an R(fx) detector is specified. What sort of SingleEllipsoidTissue could one define such that the R(fx) tally makes sense? The ellipsoid would have to be directly under the source and have close to infinite extent along the Dy dimension. Are there other cases?

dcuccia commented 6 years ago

Thanks for replying. Can't share details of what I'm working on, but R(fx) is the Fourier transform of R(x). It's general, not tied to a particular tissue's structure. If you put a line source relative to a tube, it will have an R(x) profile, or complex R(fx) phase and amplitude.

Think in general, there should be a balance of flexibility and "guardrails" in all domain tallies. Seems like we should emit text warnings for scenarios that aren't anticipated, but throwing an exception and quitting seems a bit heavy handed. It means a user can't use the Nuget package in these scenarios and will have to custom modify something (which will quickly be out of date when the library is updated).

Please don't feel like these posts need to be aggressively addressed and closed - just some "user" feedback when trying to work with the tools as provided).

On Wed, Oct 10, 2018, 1:16 PM Carole Hayakawa notifications@github.com wrote:

Hi dcuccia, I'm assuming you mean the SimulationInputValidation exception when a SingleEllipsoidTissue is specified and an R(fx) detector is specified. What sort of SingleEllipsoidTissue could one define such that the R(fx) tally makes sense? The ellipsoid would have to be directly under the source and have close to infinite extent along the Dy dimension. Are there other cases?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/VirtualPhotonics/VTS/issues/13#issuecomment-428715233, or mute the thread https://github.com/notifications/unsubscribe-auth/AAdRgYU5R1KdDCdoXZ602Tk-0f7NdpWPks5ujlWJgaJpZM4XURZz .

hayakawa16 commented 6 years ago

The infile validation checks are meant to make sure that a user specifies a combination of inputs that supports the way in which the detector tallies were coded. If the user specifies a combination of tissue/detector inputs that causes the detector tally to be incorrect, then an exception should be thrown. For example, specifying a SingleEllipsoidTissue with an ellipsoid not centered at x=0,y=0 or without Dx=Dy and using a cylindrically-symmetric detector like R(rho) would create erroneous results.

The R(fx) tally has been coded based on a Fourier transform to the RTE, not to R(x) as per Gardner's Optics Letters 36(12), 2011 paper. This means that the tally depends on the difference between the photon exit and entry locations relative to the x-axis. Implicit in this part of the tally is the assumption that the system is constant along the y-axis. The tally also depends on the photon weight which is determined by Discrete, Continuous or Analog absorption weighting methods. These vary if the tissue varies along the y-axis. Therefore to create a consistent final tally, the tissue definition needs to be constant along the y-axis.

Please let me know if my description of the R(fx) is incorrect. I do appreciate your comment about mandating rules upon the users and I don't want to be heavy-handed. Possibly the validation code should throw all warnings instead of exceptions and the users can decide if they want to proceed. I'm okay with that - I'm a linux and latex user after all ;) However, I also want to make sure that they understand that their results may in error.

I'm happy to hear the user feedback and know that you're still in the mix with us.

dcuccia commented 1 year ago

5-year-old reply is better than never, right? ;)

In my case, the simulation was asymmetric, but not erroneous - both the tissue and detector were modeling a real-life scenario. I'd agree with your suggestion that they should be warnings not errors by default. In general, it's hard to guess at a consumer's intent, and I remember this blocking my work. In order to move forward, I had to download the code and make modifications, versus live with a friendly warning that said "You're on your own with this one."

This isn't a super high priority for me, but I'd be happy to take the lead on making these changes, once the team agreed on the strategy.

hayakawa16 commented 1 year ago

That sounds great to me!

hayakawa16 commented 1 year ago

I made a fix to solve this issue. I modified the SimulationInputValidation code to allow users to specifying inhomogeneous tissue and ROfFx detector. Since the ValidationResult.IsValid now returns true, simulation will proceed without interruption. This means that the ValidationRule set to "Warning: R(fx) theory assumes a homogeneous or layered tissue geometry" and the Remarks set to "User discretion advised" are not put to the screen since only results with IsValid=false put out the Rule and Remarks statements. Please let me know any thoughts on this. Thanks!

dcuccia commented 1 year ago

Thanks Carole! Taking a step back to think about a general approach here, I think it might be a mistake to just apply this change/relaxation to one measurement domain. It's not really about "theory", it's about guiding a new user to standard use cases, or letting them relax these constraints to solve advanced problems. And that crosses all dimensions/axes. Happy to chat a little about it in person, if that's helpful. Perhaps the approach is to add a simulation option that either treats warnings as errors (the default), or skips warnings for the more advanced case.

hayakawa16 commented 1 year ago

Thanks for your reply! I see, you present an entirely orthogonal idea to one I was contemplating. That is why I put it out there! If I understand you correctly, keep SimulationInputValidation as is, i.e return IsValid is false and keep prior ValidationRule and Remarks, and put the onus on MCCL Program. Did you envision adding a Property to SimulationOptions and an overload with this property set to default of treat warnings as errors? This would not be a breaking change then. Let me know if you had another idea.

Just to put it out there, this is what I was thinking...I add an overload to ValidationResult that takes in a "int severity" parameter, with 0 being not sever, 1=warning, 2=error. IsValid is boolean so if true, severity=0, if false then severity can be 1 keep rolling but put out message, and 2 is error stop simulation. Then in MCCL Program add to IsValid=false code check on severity.

dcuccia commented 1 year ago

Something like that. Let me ponder. If you're not in a rush. I'm putting together a big proposal and want to spend some time thinking about the smoothest approach that will work across programming models. Agree with you to be non-breaking, and like the idea of error levels. Perhaps SimulationOptions.TreatValidationWarningsAsErrors (default true)...

dcuccia commented 1 year ago

Or perhaps better to create a ValidationOptions structure inside of SimulationOptions, so SimulationOptions.Validation.TreatWarningsAsErrors

hayakawa16 commented 1 year ago

Yes, let's ponder. No rush, I was just filling in between running simulations.

hayakawa16 commented 1 year ago

I've been thinking about this more. I hesitate to allow MC simulations to run if there are validation warnings because in most cases (99%), erroneous results will be generated and that is if the simulation actually finishes without fault. For example, many validation checks have to do with specifying an inclusion entirely within a layer, or specifying tissue layers that don't overlap and are contiguous, or specifying CAW and fluence or radiance detectors, etc. There are options that are not yet coded and/or assumptions in the way the code was written so that the photons move from region to region correctly. So if the user is allowed to specify SimulationOptions.Validation.TreatWarningsAsErrors as false, then these types of simulations will not run the transport correctly and most likely crash.

I just scanned the validation checks being performed in SimulationInputValidation and truly think that the ROfFx with a tissue inclusion is the only check that should be allowed at the user's risk. Can you think of another case in which we would allow a user to run with a warning? So currently, I am leaning toward solving this issue without modifying an infile option.

Let me know your thoughts.

hayakawa16 commented 10 months ago

Since my last post, I ran into a problem with how the validation software was limiting something I suggested to a user: https://groups.google.com/g/virtual-photonics/c/6aeY68AjP4c I wanted to show the user how to specify three layers, air-air-air, with middle layer surrounded by an cylindrical infinite absorber. I had to slightly modify the OPs of the middle layer to fool the validation software into thinking it was not air in order to pass the validation checks. This problem has more to do with how I coded the BoundingCylinderTissueValidation.ValidateGeometry method. However it points out a problem consistent with the ROfFx detector with heterogeneous tissue, the user wants to run something and the validation code is the limiting factor.

I'm thinking about this more. There are layers that could make this a big effort. As a first cut, I'm going to try to categorize the validation checks into two categories: 1) what will make the transport fail (e.g. a ellipsoid not entirely contained within a layer, or overlapping layer definitions), and 2) everything else. With the thought that 2) would put out a warning message but continue with the simulation and 1) would put out an error statement and stop. This would mean reviewing all validation code to see if it is coded to check for true transport errors or not and rewriting them accordingly.

Any feedback is welcome.

hayakawa16 commented 2 months ago

After working on Issue #88, I think we should keep this simple. I propose that the validation software put out warnings only. If the user specifies something that causes the transport to crash, hopefully they saw the warning. And use Console.WriteLine to output the warning. I can't rely on the ValidationResult to output anything if the check is valid.

Any thoughts?

dcuccia commented 2 months ago

Warnings sound good to me as a default.

hayakawa16 commented 2 months ago

Thank you @dcuccia for your feedback!

hayakawa16 commented 2 months ago

I fixed this similar to Issue #88. A warning is output for R(fx) detector specification in SimulationInput and tissue that is not homogeneous nor layered. Updated unit test to check for this warning and to check that validation result is true so simulation continues.

Now I think I should review other SimulationInput combinations that currently except out and instead should issue warning and continue. For example, specifying a tissue with an embedded ellipsoid off axis and a cylindrical coordinate detector.

hayakawa16 commented 2 months ago

I downgraded other SimulationInput combinations to warnings. These are ones that the user should be aware of but don't cause the transport to crash.