-
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
Cecum #76
Cecum #76
Conversation
…e d2 with curve of central path
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 good, a few points to fix, nothing major.
Also would be nice to have a rudimentary test.
@@ -35,8 +35,8 @@ def createAnnulusMesh3d(nodes, mesh, nextNodeIdentifier, nextElementIdentifier, | |||
endPointsx, endPointsd1, endPointsd2, endPointsd3, endNodeId, endDerivativesMap, | |||
forceStartLinearXi3 = False, forceMidLinearXi3 = False, forceEndLinearXi3 = False, | |||
maxStartThickness = None, maxEndThickness = None, useCrossDerivatives = False, | |||
elementsCountRadial = 1, meshGroups = None): | |||
''' | |||
elementsCountRadial = 1, meshGroups = None, tracksurface = None, startProportion = None, endProportion = None): |
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 they are a list, rename to: startProportions, endProportions
@@ -67,8 +67,12 @@ def createAnnulusMesh3d(nodes, mesh, nextNodeIdentifier, nextElementIdentifier, | |||
:param meshGroups: Optional sequence of Zinc MeshGroup for adding all new elements to, or a sequence of | |||
length elementsCountRadial containing sequences of mesh groups to add rows of radial elements to | |||
from start to end. | |||
:param tracksurface: Description for surface used for creating annulus mesh. Provides information for creating |
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.
Specify it is a representation of the outer surface.
:param startProportion: Proportion around and along of start positions on track surface. | ||
:param endProportion: Proportion around and along of end positions on track surface. |
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.
Document that these vary with nodes around as for startPoints~, endPoints~. Values only given for tracksurface for outer layer (xi3 == 1).
@@ -108,6 +112,8 @@ def createAnnulusMesh3d(nodes, mesh, nextNodeIdentifier, nextElementIdentifier, | |||
rowMeshGroups = [ meshGroups ]*elementsCountRadial | |||
else: | |||
assert len(meshGroups) == elementsCountRadial, 'createAnnulusMesh3d: Length of meshGroups sequence does not equal elementsCountRadial' | |||
if tracksurface: | |||
assert startProportion and endProportion, 'createAnnulusMesh3d: Missing start and/or end proportion for use with tracksurface' |
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.
Also assert len(startProportions) and len(endProportions) == nodesCountAround.
return annotationGroups | ||
|
||
@classmethod | ||
def generateMesh(cls, region, options): |
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 so Scaffold_base.generateMesh() is used and implement @classmethod def refineMesh(cls, meshRefinement, options) which will be similar to this function but without any call to generateBaseMesh.
I suggest doing this for all your scaffolds once you see how it works.
# if tcThickness > 0: | ||
# tubeTCWidthList = colonSegmentTubeMeshInnerPoints.getTubeTCWidthList() | ||
# xList, d1List, d2List, d3List, annotationGroups, annotationArray = getTeniaColi( | ||
# region, xList, d1List, d2List, d3List, curvatureList, tcCount, elementsCountAroundTC, | ||
# elementsCountAroundHaustrum, elementsCountAlong, elementsCountThroughWall, | ||
# tubeTCWidthList, tcThickness, sxCecum, annotationGroups, annotationArray) | ||
# | ||
# # Create flat and texture coordinates | ||
# xFlat, d1Flat, d2Flat, xTexture, d1Texture, d2Texture = createFlatAndTextureCoordinatesTeniaColi( | ||
# xiList, relaxedLengthList, cecumLength, wallThickness, tcCount, tcThickness, | ||
# elementsCountAroundTC, elementsCountAroundHaustrum, elementsCountAlong, | ||
# elementsCountThroughWall, transitElementList) | ||
# | ||
# # Create nodes and elements | ||
# nextNodeIdentifier, nextElementIdentifier, annotationGroups = createNodesAndElementsTeniaColi( | ||
# region, xList, d1List, d2List, d3List, xFlat, d1Flat, d2Flat, xTexture, d1Texture, d2Texture, | ||
# elementsCountAroundTC, elementsCountAroundHaustrum, elementsCountAlong, elementsCountThroughWall, | ||
# tcCount, annotationGroups, annotationArray, firstNodeIdentifier, firstElementIdentifier, | ||
# useCubicHermiteThroughWall, useCrossDerivatives) | ||
# | ||
# else: | ||
# # Create flat and texture coordinates | ||
# xFlat, d1Flat, d2Flat, xTexture, d1Texture, d2Texture = tubemesh.createFlatAndTextureCoordinates( | ||
# xiList, relaxedLengthList, length, wallThickness, elementsCountAround, | ||
# elementsCountAlong, elementsCountThroughWall, transitElementList) | ||
# | ||
# # Create nodes and elements | ||
# nextNodeIdentifier, nextElementIdentifier, annotationGroups = tubemesh.createNodesAndElements( | ||
# region, xList, d1List, d2List, d3List, xFlat, d1Flat, d2Flat, xTexture, d1Texture, d2Texture, | ||
# elementsCountAround, elementsCountAlong, elementsCountThroughWall, | ||
# annotationGroups, annotationArray, firstNodeIdentifier, firstElementIdentifier, | ||
# useCubicHermiteThroughWall, useCrossDerivatives) |
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 commented out code, especially if can copy from source again.
'Wall thickness']: | ||
if options[key] < 0.0: | ||
options[key] = 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.
Implement a function like that in heartatria to transfer settings (ostium wall thickness etc.) to the ostium:
@classmethod
def updateSubScaffoldOptions(cls, options):
Call this function in each of getDefaultOptions, checkOptions, generateBaseMesh as in heartatria1.
This ensures the dependent ostium options are already up-to-date.
I'm not sure if the number around can be set, but see if the logic can be moved here.
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 made the changes for ostium wall thickness, but the number of elements around ostium is only determined much later after we have created the cecum segments and identified the tracksurface where ostium sits, so I don't think it makes sense to move it to this function. Also, the number of elements around is derived and updated right before I make the ostium so it will ensure that the option is up-to-date:
ostiumSettings['Number of elements around ostium'] = len(innerEndPoints_Id)
'Tenia coli thickness': 0.5, | ||
'Wall thickness': 2.0, | ||
'Ileocecal junction': copy.deepcopy(ostiumOption), | ||
'Ileocecal junction angular position degrees': 60, |
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.
What happens if it's put over the tenia?
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 it does not happen in real life, I have an assert statement which prevents that from happening:
assert angleAroundInSector > angleToTCEdge + angleOstium * 0.5 and
angleAroundInSector < (2math.pi/tcCount) - angleToTCEdge - angleOstium0.5,
'Ileocecal junction cannot sit on tenia coli'
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 sure all asserts like this (I see there are others) are avoided in proper use by checking and limiting the value in checkOptions; asserts are exceptions which shouldn't happen by something a user enters. It's fine to have them in generateBaseMesh to catch callers who misuse the library by setting them wrong & not calling checkOptions to limit them.
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 couldn't limit the angular position of the ileocecal junction in checkOptions as I need to make the cecum segments first before I can use it to calculate the arclength of tenia coli, that's why I put it as an assert when the junction is found sitting over the tenia coli.
It's the same for the other assert which identifies insufficient elements around the ostium to create a 2-radial elements annulus. I would only know that there is insufficient elements to make that annulus after I have created a tracksurface and know where to place the ostium.
All requested changes made/addressed and added a rudimentary test for cecum. Just to let you know, there is an error in unit test for bladder which should be fixed. |
No description provided.