Feature/122 add ability to use layered concentric infinite cylinders with refractive index mismatch #133
Conversation
…inclusion tissues, I created a MultiConcentricInclusionTissue that MultiConcentricCylinderTissue can use to be created. This allows for possible MultiConcentricEllipsoidTissues to be defined in the future. Modified InfiniteCylinderTissueRegion ContainsPosiiton method to allow to very large radius cylinders (@janakarana found error). All unit tests pass. Now can work on refractive index mismatch edits.
…egion boundaries. Added comments to SingleInclusionTissue and MultiConcentricInclusionTissue describing this. Added code in MultiConcentricInclusionTissue to handle reflection and refraction. This will inturn handle MultiConcentricInfiniteCylinderTissue. All unit tests pass. Now onto writing tests to verify these code changes.
…centric-infinite-cylinders-with-refractive-index-mismatch
…oncentricInclusionTissue. Like other tissue classes that provide a general definition that can be invoked by particular classes, e.g. SingleInclusionTissue is used to create SingleEllipsoidTissue, this MultiConcentricInclusionTissue is used to create MultiConcentricInfiniteCylinderTissue. Methods like GetReflectedDirection, GetRefractedDirection are in MultiConcentricInclusionTissue and rely on accessing the SurfaceNormal from the particular inclusion class to know how to code these methods. Rearranged unit tests of class methods to match order methods are in class being tested.
…n class was added (my bad).
…inder with larger radii
…ts. Some code cleanup in InfiniteCylinderTissueRegion and CylinderTissueRegionToolbox. Corrected MultiConcentricInfiniteTissueInput Validation to NOT check for refractive index mismatch now.
…was previously written for only 2 infinite cylinders, now works for any number of cylinders. Also put back the switch statement particular case that agrees with default. Code cleanup says to remove this particular case but I think it improved readability.
…centric-infinite-cylinders-with-refractive-index-mismatch
|
I'm ready for review. |
|
I just saw sonarcloud review. Give me a bit to address these. |
… in "out" parameter since interface defines type.
|
Okay I'm ready for review. |
| ); | ||
| var result = SimulationInputValidation.ValidateInput(input); | ||
| Assert.IsFalse(result.IsValid); | ||
| } |
There was a problem hiding this comment.
This is nitpicky but I like to see a space between methods and therefore a line feed after the method and before the comments for the next method. I use space to help me find the individual methods.
| ); | ||
| var result = SimulationInputValidation.ValidateInput(input); | ||
| Assert.IsFalse(result.IsValid); | ||
| } |
| new Random()); | ||
| index = _tissue.GetNeighborRegionIndex(photon); | ||
| Assert.AreEqual(2, index); | ||
| } |
| var cosTheta = _tissue.GetAngleRelativeToBoundaryNormal(photon); | ||
| Assert.AreEqual(1,cosTheta); | ||
| } | ||
|
|
| { | ||
| FileIO.FileDelete(file); | ||
| } | ||
| } |
| var iCloned = FileIO.ReadFromJson<SingleInfiniteCylinderTissueInput>("SingleInfiniteCylinderTissue.txt"); | ||
|
|
||
| Assert.AreEqual(iCloned.Regions[1].RegionOP.Mua, i.Regions[1].RegionOP.Mua); | ||
| } |
| "SingleInfiniteCylinderTissueInput: tissue layer is assumed to be at least a single layer with air layer above and below", | ||
| "SingleInfiniteCylinderTissueInput: redefine tissue definition to contain at least a single layer of tissue"); | ||
| } | ||
| if (!tempResult.IsValid) { return tempResult; } |
There was a problem hiding this comment.
We don't need the braces here if it is on one line.
| "Resize radii and or Center so that infinite cylinders entirely within tissue layer"); | ||
| } | ||
|
|
||
| if (!tempResult.IsValid) { return tempResult; } |
There was a problem hiding this comment.
We don't need the braces if the code is on one line.
| { | ||
| // check that layer definition is valid | ||
| var tempResult = MultiLayerTissueInputValidation.ValidateLayers(layers); | ||
| if (!tempResult.IsValid){ return tempResult; } |
There was a problem hiding this comment.
We don't need the braces if the code is on one line.
| "SingleInfiniteCylinderTissueInput: infinite cylinder has radius equal to 0", | ||
| "SingleInfiniteCylinderTissueInput: make sure infinite cylinder radius is > 0"); | ||
| } | ||
| if (!tempResult.IsValid) { return tempResult; } |
There was a problem hiding this comment.
We don't need the braces if the code is on one line.
|
Hi @lmalenfant, thanks for your review. I can make these edits. |
|
I have updated code and pushed. On another topic. I pulled master and ran an infile that I ran in August 2023 and obtained identical results. Then I pulled this branch and ran the same infile. The results are not identical. I think they are statistically equivalent but not sure. I'd like to know for sure before I merge this branch into master. |
|
|
I also noticed that we cannot merge right now because I made the master branch read only. We either have to force push or remove the lock. I checked that box so we can't push to master without a PR. I need to do more research to figure out the best protection. |
|
I've performed my analysis of results using master and this branch and they agree within variance. |
|
I feel okay about merging this into master. Should I check "Merge without waiting for requirements to be met (bypass branch protections)" and click "Merge pull request"? |
|
@hayakawa16 yes go ahead and merge, I think you just need to check the box and then the button will be available. |
hayakawa16
deleted the
feature/122-add-ability-to-use-layered-concentric-infinite-cylinders-with-refractive-index-mismatch
branch
Hi, this is a draft of a PR so I can see file differences and make sure all looks good. I will assign reviewers when I'm ready for review.