Skip to content
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 urethra #85

Merged
merged 31 commits into from
Sep 25, 2020
Merged

Bladder urethra #85

merged 31 commits into from
Sep 25, 2020

Conversation

zekh167
Copy link
Contributor

@zekh167 zekh167 commented Aug 18, 2020

No description provided.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First the straightforward fixes:
There are some minor fixes in names and individual lines of code.
The separate utility functions need polishing (I'm quite careful about these).
While I think in the medium term I'd want the bladder in a separate scaffold for re-use, I think it's more important right now to update this code to use a common central path for the bladder and urethra with annotation groups marking where one stops and the other begins. We can generalise 1d path utility function extractPathParametersFromRegion to take an optional group and otherwise use it as before.

@@ -242,3 +242,31 @@ def getSurfaceProjectionAxes(ax, ad1, ad2, ad3, angle1radians, angle2radians, le
bd1 = vector.crossproduct3(ad2, bd3)
bd2 = vector.crossproduct3(bd3, bd1)
return bx, bd1, bd2, bd3

def createEllipsePoints(cx, radian, axis1, axis2, elementsCountAround, startRadians = 0.0):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is useful but it should be changed to give even spaced points in physical distance rather than radians.
Change to use:

  1. getApproximateEllipsePerimeter to get total distance around ellipse, to divide by elementsCountAround to get constant element length.
  2. updateEllipseAngleByArcLength to move the constant element length around the ellipse from the initial radians.
    Have a look at createEllipsoidPoints() for example use of the latter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still want createEllipsePoints changed as described above.

src/scaffoldmaker/utils/annulusmesh.py Outdated Show resolved Hide resolved
Comment on lines 480 to 481
for i in range(len(deleteElementIdentifier)):
elementIdentifier = deleteElementIdentifier[i]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two lines are more elegantly written in python as:

for elementIdentifier in deleteElementIdentifiers:

src/scaffoldmaker/meshtypes/meshtype_3d_bladderurethra1.py Outdated Show resolved Hide resolved
'Neck diameter 1': 5.0,
'Neck diameter 2': 3.0,
'Bladder wall thickness': 0.5,
'Neck angle': 45,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename: Neck angle degrees

bladderWallThickness = options['Bladder wall thickness']
useCrossDerivatives = options['Use cross derivatives']
useCubicHermiteThroughWall = not(options['Use linear through wall'])
neckAngle_radian = math.pi * options['Neck angle'] / 180
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaner to use this standard function:
neckAngleRadians = math.radians(options['Neck angle degrees'])
I'd also prefer to keep CamelCase rather than mix underscore style for variable names.

Comment on lines 183 to 184
'Central path LUT': copy.deepcopy(centralPathOption_LUT),
'Central path bladder': copy.deepcopy(centralPathOption_bladder),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have a discussion about how these are being used; I thought we'd have just a single path and use annotation groups to separate bladder and urethra?

'Urethra diameter 1': 1.5,
'Urethra diameter 2': 1.0,
'Urethra wall thickness': 0.5,
'Length factor': 0.5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Length factor is not self-explanatory. Perhaps will be replaced by annotation groups on central path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to "Urethra length proportion to central path". This parameter is used to identify the urethra length and also the bladder length along the central path.

'Minor diameter': 25.0,
'Neck diameter 1': 5.0,
'Neck diameter 2': 3.0,
'Bladder wall thickness': 0.5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where not having sub-scaffolds for bladder and urethra is becoming problematic since you have to be clear about which part the parameter belongs to. Perhaps make it a rule that since this is 'Bladder with Urethra', bladder-only parameters don't need to be qualified, but you need to put 'Urethra' in front of urethra-specific parameters.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want you to change to annotate the central path and have explained how in the comments.
It's really much better as user can make a path of any length provided they make appropriate groups for 'urinary bladder' and 'urethra', and can guarantee have a node on the junction. Remove the length/proportion.
Note: I have just made some necessary fixes to scaffoldmaker and meshgeneratorstep so you'll need to update the codes first.
Remember to also check that your bladder test passes - may already have mismatches in numbers of annotation groups. May want to test the subscaffold annotation groups exists too?

@@ -491,7 +454,9 @@ def generateBaseMesh(cls, region, options):
d2 = [segmentLength, 0.0, 0.0]
elif n1 == elementsCountAlongBladder:
if not includeUrethra:
d2 = [0.0, 0.0, segmentLength]
badderSegmentLength = (1 - lengthFactor) * length / elementsCountAlongBladder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: badder

[[0.3, 3.2, 75.2], [0.5, 1.5, 12.7], [0.0, 0.5, 0.0], [0.0, 0.0, -0.5]],
[[0.6, 5.6, 90.2], [0.5, 1.8, 13.2], [0.0, 0.5, 0.0], [0.0, 0.0, -0.5]],
[[1.0, 8.7, 105.0], [0.7, 3.7, 12.9], [0.0, 0.5, 0.0], [0.0, 0.0, -0.5]],
[[1.7, 13.9, 119.2], [0.9, 5.5, 13.3], [0.0, 0.5, 0.0], [0.0, 0.0, -0.5]]])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wanting you to add user annotation groups demarking which part of the path is bladder and which is urethra, for both cat and rat.
Add a comma after the above line and add (though not perfectly elegant with double calls to get_bladder_term):

            'userAnnotationGroups': [
                {
                    '_AnnotationGroup': True,
                    'dimension': 1,
                    'identifierRanges': '1-4',
                    'name': get_bladder_term('urinary bladder')[0],
                    'ontId': get_bladder_term('urinary bladder')[1]
                },
                {
                    '_AnnotationGroup': True,
                    'dimension': 1,
                    'identifierRanges': '5-8',
                    'name': get_bladder_term('urethra')[0],
                    'ontId': get_bladder_term('urethra')[1]
                } ]

To use these we'll add an optional group name parameter to extractPathParametersFromRegion in meshtype_1d_path1.py:

def extractPathParametersFromRegion(region, groupName = None):
    '''
    :param groupName: Optional name of group to get node parameters from, otherwise all nodes are used.
    ''''
    # at the appropriate point after:
    nodes = fm.findNodesetByFieldDomainType(Field.DOMAIN_TYPE_NODES)
    if groupName:
        group = fm.findFieldByName(groupName).castGroup()
        nodes = group.getFieldNodeGroup(nodes).getNodesetGroup()
        assert nodes.isValid()  # in case invalid or not found

You'll need to update the bladder with urethra to get parameters from the path with the appropriate group name matching the annotation group for bladder or urethra as we added earlier (from bladder terms).

'Urethra diameter 1',
'Urethra diameter 2',
'Urethra wall thickness',
'Urethra length proportion to central path',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This length proportion is no longer needed with an annotated central path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After deleting the "Urethra length proportion to central path", Should I delete the "Number of elements Along" from options and instead add "Number of elements Along Bladder" and "Number of elements Along Urethra", which are independent?
Then, the segmentLength will be different in bladder and urethra parts, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you'll have independent numbers of elements along bladder and urethra, but you need to make the element size transition properly between them. Either the urethra fits to the bladder or vice versa.

# Central path
tmpRegion = region.createRegion()
centralPath.generate(tmpRegion)
cx, cd1, cd2, cd12 = extractPathParametersFromRegion(tmpRegion)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be called twice to extract parameters for bladder and urethra separately, passing the group name. See comment earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change the following part, too? Because the segmentLength will be different after deleting the "urethraLengthPortion".

    # Find arcLength
    length = 0.0
    elementsCountIn = len(cx) - 1
    sd1 = interp.smoothCubicHermiteDerivativesLine(cx, cd1, fixAllDirections=True,
                                                   magnitudeScalingMode=interp.DerivativeScalingMode.HARMONIC_MEAN)
    for e in range(elementsCountIn):
        arcLength = interp.getCubicHermiteArcLength(cx[e], sd1[e], cx[e + 1], sd1[e + 1])
        length += arcLength
    segmentLength = length / elementsCountAlong

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need to smooth the central path. (Separately we need to find a way for the user to smooth the 1D path as needed; on the to-do list.)
You should have a separate cx, cd1 for bladder and urethra. Can have a fixed segment length for one or the other, but not both:
If it is the bladder, the first element of the urethra needs to transition from the derivatives supplied by the end of the bladder. If it is the urethra, the last element of the bladder needs to transition to fit the first urethra derivatives.

Comment on lines 351 to 364
# Sample central path
# sx, sd1, se, sxi, ssf = interp.sampleCubicHermiteCurves(cx, cd1, elementsCountAlong)
# sd2, sd12 = interp.interpolateSampleCubicHermite(cd2, cd12, se, sxi, ssf)
if includeUrethra:
sx, sd1, se, sxi, ssf = interp.sampleCubicHermiteCurves(cx, cd1, elementsCountAlong)
sd2, sd12 = interp.interpolateSampleCubicHermite(cd2, cd12, se, sxi, ssf)
else:
m = int((1 - urethraproportion) * len(cx))
cx = cx[:m+1]
cd1 = cd1[:m+1]
cd2 = cd2[:m+1]
cd12 = cd12[:m+1]
sx, sd1, se, sxi, ssf = interp.sampleCubicHermiteCurves(cx, cd1, elementsCountAlongBladder)
sd2, sd12 = interp.interpolateSampleCubicHermite(cd2, cd12, se, sxi, ssf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed with annotated central path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I delete this part, how I can run the following part?

    # Project reference point for warping onto central path
    sxRefList, sd1RefList, sd2ProjectedListRef, zRefList = \
         tubemesh.getPlaneProjectionOnCentralPath(innerNodes_x, elementsCountAround, elementsCountAlong,
                                                  length, sx, sd1, sd2, sd12) 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still do this on each respective sets of cx, cd1 for the bladder and urethra.

Comment on lines 9 to 12
from opencmiss.utils.zinc.general import ChangeManager
from opencmiss.zinc.element import Element
from opencmiss.zinc.field import Field
from opencmiss.zinc.field import Field, FieldGroup
from opencmiss.zinc.node import Node
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the remnants to your changes to annulus mesh since you've moved element/node deletion code elsewhere.

Copy link
Member

@rchristie rchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good.
At some future point we may want to use some of the new smoothing/normal functions of 1d path to improve default central paths.

@rchristie rchristie merged commit b49717d into ABI-Software:master Sep 25, 2020
@zekh167 zekh167 deleted the bladder_urethra branch September 27, 2020 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants