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

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

Closed
dcuccia opened this issue Oct 9, 2018 · 18 comments · Fixed by #162
Closed

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

dcuccia opened this issue Oct 9, 2018 · 18 comments · Fixed by #162
Assignees

Comments

@dcuccia
Copy link
Contributor

dcuccia commented Oct 9, 2018

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
Copy link
Member

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
Copy link
Contributor Author

dcuccia commented Oct 10, 2018 via email

@hayakawa16
Copy link
Member

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
Copy link
Contributor Author

dcuccia commented Jul 27, 2023

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
Copy link
Member

That sounds great to me!

@hayakawa16
Copy link
Member

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
Copy link
Contributor Author

dcuccia commented Sep 13, 2023

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
Copy link
Member

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
Copy link
Contributor Author

dcuccia commented Sep 14, 2023

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
Copy link
Contributor Author

dcuccia commented Sep 14, 2023

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

@hayakawa16
Copy link
Member

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

@hayakawa16
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

dcuccia commented Aug 7, 2024

Warnings sound good to me as a default.

@hayakawa16
Copy link
Member

Thank you @dcuccia for your feedback!

@hayakawa16
Copy link
Member

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
Copy link
Member

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.

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 a pull request may close this issue.

2 participants