-
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
Vary inner radius and tenia coli width along colon length #59
Conversation
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've demonstrated that the bulk of the work to get proper variation across segments is working fine; I've not looked into that in details here.
The code around this needs some changes:
Colon-specific parameters are being passed to tubemesh; as described in the comments, anything specific to a particular segment profile needs to be supplied to and sampled by the profile, not tubemesh itself.
Best have a discussion with me; it might simplify things.
I'm also wondering about removing the 'simple mesentery' case and just consider it as being a colon with a single tenia coli and no extra thickness (i.e. no extra later of elements).
xList = [] | ||
d1List = [] | ||
for i in range(len(lengthList)): | ||
v = [lengthList[i], paramList[i], 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.
No need for 3rd component for v and d1 as smooth function handles n components.
d1 = [(lengthList[i+1] - lengthList[i]) if i < len(lengthList)-1 else (lengthList[i] - lengthList[i-1]), 0.0, 0.0] | ||
nx.append(v) | ||
nd1.append(d1) | ||
smoothd1 = interp.smoothCubicHermiteDerivativesLine(nx, nd1) |
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.
Alternatively, could have proximal, (mid) transverse and distal parameters and assume zero derivatives? That would make this function colon-specific. What are researchers likely to measure?
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.
Discussed offline with Richard. Decided to keep things as they are now and will modify later when requirements change.
@@ -350,3 +453,59 @@ def generateMesh(cls, region, options): | |||
meshrefinement = MeshRefinement(baseRegion, region, baseAnnotationGroups) | |||
meshrefinement.refineAllElementsCubeStandard3d(refineElementsCountAround, refineElementsCountAlong, refineElementsCountThroughWall) | |||
return meshrefinement.getAnnotationGroups() | |||
|
|||
def sampleParameterAlongLength(paramList, lengthList, segmentCount): |
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.
As written this function is general purpose so could go in tubemesh? But read comments below first...
Update: sampling needs to be done in the tube mesh segment generator as described elsewhere.
|
||
# Find parameter value at each segment | ||
xTol = 1.0E-6 | ||
for n in range(segmentCount + 1): |
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.
Isn't this loop the same as interp.sampleCubicHermiteCurves (also interp.sampleCubicHermiteCurvesSmooth)? These would be more efficient than using getCubicHermiteCurvesPointsAtArcDistance...
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.
interp.sampleCubicHermiteCurves will sample the curve describing how parameter changes along the centre line into equal arclength, but here I want to sample along the center line and get the parameter value along each of these points on the center line.
'Inner radius': 0.2, | ||
'Mesenteric zone width': 0.08, | ||
'Start inner radius': 0.094, | ||
'Start radius derivative': 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.
Start inner radius derivative, also End below
""" | ||
|
||
def __init__(self, region, elementsCountAroundMZ, elementsCountAroundNonMZ, | ||
elementsCountAlongSegment, segmentLength, wallThickness): |
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.
indent second row of arguments
mzWidth, radius, segmentLength, wallThickness): | ||
class TubeMeshSegmentInnerPointsNoTeniaColi: | ||
""" | ||
Generates a class object and function to pass the inner profile |
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.
Generates inner profile of a simple mesentery colon segment i.e. no tenia coli, for use by tubemesh.
cornerInnerRadiusFactor, haustrumInnerRadiusFactor, segmentLengthEndDerivativeFactor, segmentLengthMidDerivativeFactor, segmentLength, wallThickness): | ||
class TubeMeshSegmentInnerPointsTeniaColi: | ||
""" | ||
Generates a class object and function to pass the inner profile |
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.
Improve description - see simple mesentery comment
scaffoldmaker/utils/tubemesh.py
Outdated
xInner, d1Inner, d2Inner, wallThickness, | ||
segmentAxis, segmentLength, | ||
innerRadiusList, dInnerRadiusList, | ||
tcWidthList, dTCWidthList, |
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.
Can't pass colon-specific parameters in this way as it takes away the generic-ness of tubemesh.
All varying properties need to be passed to the segment generator, including radius and TC width. This makes sense as they are specific to that generator.
Tubemesh will supply distance along when it calls the segment generator which can use it to sample the varying properties.
annotationGroups, annotationArray, transitElementList, uList, arcLengthOuterMidLength) | ||
annotationGroups, nextNodeIdentifier, nextElementIdentifier, xList, d1List, d2List, d3List, sx, curvatureAlong, factorList, uList, relaxedLengthList, tubeTCWidthList = tubemesh.generatetubemesh(region, | ||
elementsCountAround, elementsCountAlongSegment, elementsCountThroughWall, segmentCount, cx, cd1, cd2, cd12, innerRadiusSegmentList, dInnerRadiusSegmentList, | ||
tcWidthSegmentList, dTCWidthSegmentList, tubeMeshSegmentInnerPoints, wallThickness, segmentLength, useCrossDerivatives, useCubicHermiteThroughWall) |
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.
Can't pass TC width here - see comments on tubemesh class.
I've made the changes as per request, please review. Thanks! |
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.
There are values returned by tubemesh (tubeTCWidthList etc.) which it shouldn't know about, making it not generic... see comments.
Also request change to parameter sampling code - should be equally spaced along length.
elementsCountAround, elementsCountAlongSegment, elementsCountThroughWall, segmentCount, cx, cd1, cd2, cd12, | ||
xInner, d1Inner, d2Inner, wallThickness, segmentAxis, segmentLength, useCrossDerivatives, useCubicHermiteThroughWall, | ||
annotationGroups, annotationArray, transitElementList, uList, arcLengthOuterMidLength) | ||
annotationGroups, nextNodeIdentifier, nextElementIdentifier, xList, d1List, d2List, d3List, sx, curvatureAlong, factorList, uList, relaxedLengthList, tubeTCWidthList = tubemesh.generatetubemesh(region, |
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.
Can't return anything specific to a segment profile from this function either (relaxedLengthList, tubeTCWidthList; not sure what ulist is). Probable solution is to store these in the tubeMeshSegmentInnerPoints? Alternative is to return a generic map of names to lists of values.
|
||
# Generate tenia coli | ||
if segmentScaffoldType == MeshType_3d_colonsegmentteniacoli1: | ||
if tcCount > 1: |
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 a boolean flag setting control whether these are made, or zero thickness.
useCrossDerivatives, useCubicHermiteThroughWall, xList, d1List, d2List, d3List, | ||
elementsCountAroundTC, elementsCountAroundHaustrum, elementsCountAlongSegment, elementsCountThroughWall, wallThickness, | ||
tcWidth, tcThickness, sx, curvatureAlong, factorList, uList, arcLengthOuterMidLength, segmentLength, tcCount) | ||
if tcCount > 1: |
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.
See previous comment for colon 1. Replace with flag or zero thickness.
Generates a 3-D colon segment mesh with two or three tenia coli with variable | ||
|
||
def __init__(self, region, elementsCountAroundTC, elementsCountAroundHaustrum, | ||
elementsCountAlongSegment, tcCount, segmentLengthEndDerivativeFactor, |
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.
indent argument lines 2 onwards with 8 spaces (2*4). See second accepted example at https://www.python.org/dev/peps/pep-0008/#code-lay-out.
endTCWidth = self._tcWidthSegmentList[nSegment+1] | ||
endTCWidthDerivative = self._dTCWidthSegmentList[nSegment+1] | ||
|
||
return getColonSegmentInnerPoints(self._region, self._elementsCountAroundTC, |
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.
Expect some of the return values from this call to be held here and not returned to tubemesh, e.g. TC widths.
innerRadiusSegmentList, dInnerRadiusSegmentList = interp.sampleParameterAlongCenterLine(innerRadiusList, lengthList, segmentCount) | ||
|
||
tcWidthList = [proximalTCWidth, proximalTransverseTCWidth, transverseDistalTCWidth, distalTCWidth] | ||
tcWidthSegmentList, dTCWidthSegmentList = interp.sampleParameterAlongCenterLine(tcWidthList, lengthList, segmentCount) |
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 had expected the sampling to be done within the TubeMeshSegmentInnerPoints, but I see you instead pass the segment number when calling it. I'm happy with this.
scaffoldmaker/utils/interpolation.py
Outdated
@@ -787,3 +787,61 @@ def getDoubleCubicHermiteCurvesMidDerivative(ax, ad1, mx, bx, bd1): | |||
magb = vector.magnitude(bd1) | |||
magm = arcLengtha + arcLengthb - 0.5*(maga + magb) | |||
return vector.setMagnitude(md1, magm) | |||
|
|||
def sampleParameterAlongCenterLine(paramList, lengthList, outputElementsCount): |
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.
Remove 'Center' from name as redundant (I try to avoid using names where there are US spelling differences anyway). I would pass the lengthList as first argument because it is the reference onto which the parameters are mapped (the x to the y).
scaffoldmaker/utils/interpolation.py
Outdated
nd1.append(d1) | ||
smoothd1 = smoothCubicHermiteDerivativesLine(nx, nd1) | ||
|
||
# Get total arclength |
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 should be sampling at equal distances along the lengths, not including the parameter as a component of coordinates (it may after all be used for a non-spatial value). It would look increasingly wrong if there was a lot of variation of the parameter at one end vs. the other.
I think we need a special smoothing function to get derivatives of parameter p with linearly varying x lengths.
The arc length should just be the total length of the line (last x - first x). Find xi in the original elements at sample lengths, then evaluate p, dp/dxi, dx/dxi and use chain rule to get dp/dx. Divide by outputElementsCount to get derivatives for the segment length?
Probably easier to describe on paper.
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.
Minor changes now. A rename and a new flag option.
|
||
relaxedLengthList, xiList = tubeMeshSegmentInnerPoints.getRelaxedLengthAndXiList() | ||
|
||
if tcCount != 1: |
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 test a separate setting e.g. a boolean flag indicating extra tenia thickness (or possibly if that thickness > 0). Should be possible to make 2, 3 tenia without extra thickening. Just as importantly, it means the code is self-documenting.
tubeMeshSegmentInnerPoints = TubeMeshSegmentInnerPoints( | ||
region, elementsCountAroundTC, elementsCountAroundHaustrum, elementsCountAlongSegment, | ||
tcCount, segmentLengthEndDerivativeFactor, segmentLengthMidDerivativeFactor, | ||
segmentLength, wallThickness, cornerInnerRadiusFactor, haustrumInnerRadiusFactor, | ||
innerRadiusSegmentList, dInnerRadiusSegmentList, tcWidthSegmentList, dTCWidthSegmentList) |
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 object has a generic name but takes TC arguments?
@@ -6,13 +6,13 @@ | |||
|
|||
import copy | |||
from scaffoldmaker.meshtypes.meshtype_1d_path1 import MeshType_1d_path1, extractPathParametersFromRegion | |||
from scaffoldmaker.meshtypes.meshtype_3d_colonsegmentsimplemesentery1 import MeshType_3d_colonsegmentsimplemesentery1, getColonSegmentInnerPointsNoTeniaColi | |||
from scaffoldmaker.meshtypes.meshtype_3d_colonsegmentteniacoli1 import MeshType_3d_colonsegmentteniacoli1, getColonSegmentInnerPointsTeniaColi, getTeniaColi | |||
from scaffoldmaker.meshtypes.meshtype_3d_colonsegment1 import MeshType_3d_colonsegment1, TubeMeshSegmentInnerPoints, getTeniaColi, createFlatAndTextureCoordinatesTeniaColi, createNodesAndElementsTeniaColi |
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.
TubeMeshSegmentInnerPoints name is too generic; should be renamed to describe that it's for colon only. Probably want to rename the original class in this case, but worth knowing you can always rename something on import:
from scaffoldmaker.meshtypes.meshtype_3d_colonsegment1 import TubeMeshSegmentInnerPoints as ColonSegmentTubeMeshSegmentInnerPoints
This is often needed to disambiguate objects with the same name in two sources.
No description provided.