-
Notifications
You must be signed in to change notification settings - Fork 35
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
Bladder coordinates #235
Bladder coordinates #235
Conversation
…present an ellipsoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed serosa/lumen annotations were reversed, but the problem is caused by incorrect extrusion from outer surface (use Hermite through wall to see D3 is inward; I need to see why the element axes are not showing the problem). See comment in code for fixing - should be straightforward.
See also comment about hiding fixed ureter position option.
Please be very careful to check annotations and tests are fine.
Other than that I'm very happy with the material coordinates.
@@ -1297,7 +1335,7 @@ def getBladderCoordinates(elementsCountAlongDome, elementsCountAlongNeck, elemen | |||
d3Inner.append(d3) | |||
|
|||
# Create outer layers from the inner nodes | |||
wallThicknessList = [wallThickness] * (elementsCountAlongBladder + 1) | |||
wallThicknessList = [-wallThickness] * (elementsCountAlongBladder + 1) | |||
relativeThicknessList = [] | |||
transitElementList = [0] * elementsCountAround | |||
xList, d1List, d2List, d3List, curvatureList = \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just negate the wall thickness and pass to this function. What you're getting is elements with negative volume, with xi0 on the outside and xi1 on the inside. This also means your lumen/serosa annotations appear wrong.
Note the surface annotation code is correct: you need to fix this function to be able to create coordinates from outer properly.
I suggest you rename Mabelle's tubemesh function to extrudeSurfaceCoordinates() and have an additional option outward=True (you will supply False, which will internally use -wall thickness, but importantly must keep layer ordering from inside to out and d3 direction from inside to out. This should be a small change to this function; I don't want to see a whole copy of the code for the outward=False case, should just be a different sequence of inserting in the lists, e.g. list.insert(0, layer) for that case, where list.append(layer) is used for outward=True.
Please be careful to rename all variables so they say e.g. xSurf rather than xInner and ensure the function arguments are correctly documented.
Update comment to say 'create inner layers from outer' in bladder.
Please make sure all the tests pass and the annotations look correct. Check Colon etc. look correct and its annotations are correct. Be careful that all previous uses of getCoordinateFromInner call the new function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I renamed Mabelle's tubemesh function and added the outward parameter to this function in all previous uses. Nothing else has been changed for the other scaffolds' code.
- I also changed all variables in that function in tubemesh and documented the function arguments.
- Changed the tubemesh function in order to create the nodes order from inner to outer, d3 pointing from inside to out while the bladder scaffold is created inward (outward=False). It also works properly for the outward=True in other scaffolds like colonsegment and esophagus.
- Adjusted the markers location.
- Checked the annotation to be true.
- Bladder test has been updated. All tests pass.
options['Ureter position around'] = 0.67 # should be on the dorsal part (> 0.5) | ||
if 'Material' in parameterSetName: | ||
options['Number of elements along dome'] = 8 | ||
options['Number of elements along neck'] = 4 | ||
options['Number of elements around'] = 8 | ||
options['Wall thickness'] = 0.01 | ||
options['Wall thickness'] = 0.016 # an average of the wall thicknesses for above species | ||
options['Ureter position around'] = 0.67 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we require the 'Ureter position around' to be the same for all variants, remove it from the per-species and material code. Add a comment explaining that it's a fixed material coordinate and thus not to be edited.
Remove this option from the visible options in getOrderedOptionNames() below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as suggested above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor fixes and code improvements.
Please eyeball changes and run tests again after changing.
markerList = [] | ||
markerList.append({"group": apexGroup, "elementId": idx1, "xi": xi1}) | ||
idx2 = elementsCountAlongDome * elementsCountAround * elementsCountThroughWall + ureterElementPositionAround + 1 | ||
xi2 = [ureter1Position.xi1, 0.0, 1.0] | ||
xi2 = [ureter1Position.xi1, 0.0, 0.0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have remained at 1.0 ? Want apex marker on the outside. Check it is correct and the bladder test still passes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the apex to outside surface and updated the test. Done.
@@ -344,7 +344,7 @@ def getDefaultOptions(cls, parameterSetName='Default'): | |||
'Number of elements around': 8, | |||
'Number of elements through wall': 1, | |||
'Wall thickness': 0.016, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default parameters use the cat central path but the material wall thickness - please change to make cat parameters the default. Just need to change wall thickness to 0.007 above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if 'Rat 1' in parameterSetName: | ||
options['Wall thickness'] = 0.006 # was 0.3 * 2.0 / 110.40996641878101 | ||
options['Ureter position around'] = 0.67 # should be on the dorsal part (> 0.5) | ||
if 'Material' in parameterSetName: | ||
options['Number of elements along dome'] = 8 | ||
options['Number of elements along neck'] = 4 | ||
options['Number of elements around'] = 8 | ||
options['Wall thickness'] = 0.016 # an average of the wall thicknesses for above species |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make the wall thickness in material coordinates 0.02 so it's a nice round number (and closer to half the human wall thickness). I'm still surprised by the variations here...
options['Wall thickness'] = 0.02 # an average of the wall thicknesses for above species, rounded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
:param xSurf: Coordinates on surface | ||
:param d1Surf: Derivatives on surface around tube | ||
:param d2Surf: Derivatives on surface along tube | ||
:param d3Surf: Derivatives on surface through wall | ||
:param wallThicknessList: Wall thickness for each element along tube |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually for each node along tube?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this represents the wallthickness for each layer of elements along tube.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code says:
for n2 in range(elementsCountAlong + 1):
wallThickness = wallThicknessList[n2]
which would make it nodes.
:param wallThicknessList: Wall thickness for each element along tube | ||
:param relativeThicknessList: Relative wall thickness for each element through wall | ||
:param elementsCountAround: Number of elements around tube | ||
:param elementsCountAlong: Number of elements along tube | ||
:param elementsCountThroughWall: Number of elements through tube wall | ||
:param transitElementList: stores true if element around is a transition | ||
element that is between a big and a small element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restore this comment line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored.
src/scaffoldmaker/utils/tubemesh.py
Outdated
if outward: | ||
wallThickness = wallThicknessList[n2] | ||
else: | ||
wallThickness = -wallThicknessList[n2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code below has several near identical cases for inward and outward which are different only because of the above sign. Don't modify the wallThickness, instead have a new variable:
wallOutwardDisplacement = wallThickness if outward else -wallThickness
Use it in the extrusion code below (see other comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/scaffoldmaker/utils/tubemesh.py
Outdated
xOuter.append(x) | ||
norm = d3Surf[n] | ||
# Calculate extruded coordinates | ||
x = [xSurf[n][i] + norm[i]*wallThickness for i in range(3)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the signed wall displacement here only:
x = [xSurf[n][i] + norm[i]*wallOutwardDisplacement for i in range(3)]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/scaffoldmaker/utils/tubemesh.py
Outdated
if outward: | ||
dWall = [wallThickness * c for c in norm] | ||
x = interp.interpolateCubicHermite(surfx, dWall, extrudedx, dWall, xi3) | ||
else: | ||
dWall = [-wallThickness * c for c in norm] | ||
x = interp.interpolateCubicHermite(extrudedx, dWall, surfx, dWall, xi3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need separate cases for x = ~, but dWall = should not depend on inward/outward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Kept the dWall the same, but different x.
All above comments applied and updated the bladder, bladderurethra, cecum, colon, colonsegment, esophagus and smallintestine tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d1 calculation is incorrect in extrusion.
Fix comment.
cecum, colon, colon segment, esophagus and small intestine test answers should not have changed - please restore them.
:param xSurf: Coordinates on surface | ||
:param d1Surf: Derivatives on surface around tube | ||
:param d2Surf: Derivatives on surface along tube | ||
:param d3Surf: Derivatives on surface through wall | ||
:param wallThicknessList: Wall thickness for each element along tube |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code says:
for n2 in range(elementsCountAlong + 1):
wallThickness = wallThicknessList[n2]
which would make it nodes.
src/scaffoldmaker/utils/tubemesh.py
Outdated
xList.append(x) | ||
|
||
# dx_ds1 | ||
factor = 1.0 + wallThickness*xi3 * curvatureAroundInner[n] | ||
d1 = [ factor*c for c in d1Inner[n]] | ||
factor = 1.0 - wallOutwardDisplacement * xi3 * curvatureAroundSurf[n] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code was correct (except for rename of Inner -> Surf):
factor = 1.0 + wallThickness * xi3 * curvatureAroundSurf[n]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix d3 in tubemesh
src/scaffoldmaker/utils/tubemesh.py
Outdated
d2List.append(d2) | ||
curvatureList.append(curvature) | ||
|
||
#dx_ds3 | ||
d3 = [c * wallThickness * (relativeThicknessList[n3] if relativeThicknessList else 1.0/elementsCountThroughWall) for c in norm] | ||
d3 = [c * -wallOutwardDisplacement * (relativeThicknessList[n3] if relativeThicknessList else 1.0 / elementsCountThroughWall) for c in norm] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old code was fine. Use wallThickness not -wallOutwardDisplacement which was reversing colon d3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great now.
Hi Richard. I made the following changes based on our last discussion: