Revisions based on comments from Colon Pull request #43 #44
Conversation
There was a problem hiding this comment.
'Haustra 1' scaffold should also be generated using tube mesh, which removes a lot of code.
I've also asked to rename 4 parameters to consistently use the terms 'haustrum' and 'haustrum length' when referring to parameters that apply to each haustrum. Was a mix of haustra and haustrum and segment.
With these fixes it's ready to pull in.
| 'Inner radius': 0.5, | ||
| 'Corner inner radius factor': 0.5, | ||
| 'Haustra inner radius factor': 0.5, | ||
| 'Haustrum segment end-derivative factor': 0.5, |
There was a problem hiding this comment.
Rename 2 options:
'Haustrum length end derivative factor'.
'Haustrum length mid derivative factor'.
Decided best to use 'length' to relate to 'Haustrum length' parameter below, and have gone cold on using hyphen.
| 'Haustrum segment end-derivative factor': 0.5, | ||
| 'Haustrum segment mid-derivative factor': 1.0, | ||
| 'Wall thickness': 0.01, | ||
| 'Haustra segment length': 1.0, |
| 'Number of elements through wall' : 1, | ||
| 'Inner radius': 0.5, | ||
| 'Corner inner radius factor': 0.5, | ||
| 'Haustra inner radius factor': 0.5, |
There was a problem hiding this comment.
'Haustrum inner radius factor'.
I'm happy to call the whole scaffold 'Haustra 1' since it represents a segment of colon with 3 Haustra.
However, better if parameters are consistent about sizing each haustrum.
There are 4 option names changes ... be careful to update all uses and check both scaffolds run.
| haustrumSegmentEndDerivativeFactor = options['Haustrum segment end-derivative factor'] | ||
| haustrumSegmentMidDerivativeFactor = options['Haustrum segment mid-derivative factor'] | ||
| wallThickness = options['Wall thickness'] | ||
| haustraSegmentLength = options['Haustra segment length'] |
There was a problem hiding this comment.
Make variables consistent with options... sorry!
haustrumLength = options['Haustrum length']
| useCubicHermiteThroughWall = not(options['Use linear through wall']) | ||
| elementsCountAlong = elementsCountAlongHaustrum | ||
|
|
||
| nodeIdentifier = 1 |
There was a problem hiding this comment.
The remainder of this function should call getColonHaustraSegmentInnerPoints and then generate with the tube mesh - copy the code from 'Colon 1'.
You just have to make a centre line:
x = [ [ 0.0, 0.0, 0.0 ], [ haustrumLength, 0.0, 0.0 ] ]
d1 = [ [ haustrumLength, 0.0, 0.0 ], [ haustrumLength, 0.0, 0.0 ] ]
| options['Number of elements around'] = 15 | ||
| options['Number of elements along haustrum'] = 4 | ||
| options['Inner radius'] = 1.0 | ||
| options['Haustrum segment mid-derivative factor'] = 2.0 |
There was a problem hiding this comment.
Rename consistently with 'Haustra 1' scaffold... see later.
'Haustrum length mid derivative factor'
| @staticmethod | ||
| def getOrderedOptionNames(): | ||
| optionNames = MeshType_3d_haustra1.getOrderedOptionNames() | ||
| optionNames.remove('Haustra segment length') |
| haustraSegmentCount = options['Number of haustra segments'] | ||
| radius = options['Inner radius'] | ||
| cornerInnerRadiusFactor = options['Corner inner radius factor'] | ||
| haustraInnerRadiusFactor = options['Haustra inner radius factor'] |
There was a problem hiding this comment.
Carefully update option names and variable names - see comments in 'Haustra 1' scaffold.
| for e in range(elementsCountIn): | ||
| arcLength = getCubicHermiteArcLength(cx[e], sd1[e], cx[e + 1], sd1[e + 1]) | ||
| length += arcLength | ||
| haustraSegmentLength = length / haustraSegmentCount |
|
I've made the requested changes and tested on both haustra and colon scaffolds to ensure they are running properly. |
Pull request #43 will be cancelled in view of revisions made on new branch.