Material coordinates - #295
Conversation
rchristie
left a comment
rchristie
left a comment
There was a problem hiding this comment.
Not really a full review as this is a larger change than I expected (which was just to add material coordinates). For a larger change including upper/lower limbs etc. I will need to look at when I'm back from Mon 17.
There are some comments about unnecessary formatting changes, but little real substance due to the following:
For some reason the meshtype_3d_wholebody2 scaffold diff is unable to be rendered in the github interface. Has it caught up to the tube mesh dome changes?
I'll need you to walk me through the changes now they're a bit larger.
| _allScaffoldTypes = [ | ||
| MeshType_1d_bifurcationtree1, | ||
| MeshType_1d_uterus_network_layout1, | ||
| MeshType_1d_human_body_network_layout1, |
There was a problem hiding this comment.
This shouldn't be committed to the public scaffold list.
I don't expect any change to this whole file -- can you revert it?
| AnnotationGroup, | ||
| findOrCreateAnnotationGroupForTerm, | ||
| getAnnotationGroupForTerm, | ||
| ) |
There was a problem hiding this comment.
Don't needlessly change style like this. I don't want one line per imported function as we can get dozens from some utilities.
| import math | ||
|
|
There was a problem hiding this comment.
math was already being imported below -- don't change unlesss we agree on a new standard for ordering, and the current approach is that python system libraries are imported last after our own sources.
| computeCubicHermiteEndDerivative, getCubicHermiteArcLength, interpolateLagrangeHermiteDerivative, | ||
| sampleCubicHermiteCurvesSmooth, smoothCubicHermiteDerivativesLine) | ||
| computeCubicHermiteEndDerivative, | ||
| getCubicHermiteArcLength, | ||
| interpolateLagrangeHermiteDerivative, | ||
| sampleCubicHermiteCurvesSmooth, | ||
| smoothCubicHermiteDerivativesLine, | ||
| ) | ||
| from scaffoldmaker.utils.networkmesh import NetworkMesh | ||
| from scaffoldmaker.utils.tubenetworkmesh import BodyTubeNetworkMeshBuilder, TubeNetworkMeshGenerateData | ||
| import math | ||
| from scaffoldmaker.utils.tubenetworkmesh import ( | ||
| BodyTubeNetworkMeshBuilder, | ||
| TubeNetworkMeshGenerateData, | ||
| ) |
There was a problem hiding this comment.
Revert these unnecessary changes.
| ("abdominopelvic cavity", "UBERON:0035819"), | ||
| ("upper limb", "UBERON:0001460"), | ||
| ("left upper limb", "UBERON:8300002", "FMA:7186"), | ||
| ("left shoulder", ""), |
There was a problem hiding this comment.
Do any of these new terms have ILX or UBERON ids?
Look up on interlex...
| ("head core", ""), | ||
| ("diaphragm", "UBERON:0001103", "ILX:0103194"), | ||
| ("hand", "ILX:0104885", "FMA:9712"), | ||
| ("hip", ""), |
There was a problem hiding this comment.
Does hip have a real anatomical definition? Perhaps needs to be a more focused term for the level of the top of the pelvis?
| @@ -0,0 +1,139 @@ | |||
| #%% | |||
There was a problem hiding this comment.
I wouldn't have started a new utility that is used in one place, but it's OK.
However, need to have header text explaining what it is for.
| 'footElementsCount': 2 | ||
| } | ||
|
|
||
| def create_segment_layout(nodeCount:int, nodeIdentifier:int, |
There was a problem hiding this comment.
This is a very generic name. Is the function able to be used for other scaffolds?
It seems like it should be a method of a class that maintains the next node identifier...
|
|
||
| :param nodeCount: Number of nodes to add. | ||
|
|
||
| :param networkLayout: String containing the current network layout. |
There was a problem hiding this comment.
No such argument. A NetworkLayout is a specific class. This would logically be what we call a structure string.
| @@ -0,0 +1,139 @@ | |||
| #%% | |||
| # Number of elements per segment. Used to calculate the number of nodes per segment. | |||
| human_network_element_counts = { | |||
There was a problem hiding this comment.
This might be better done as a members of a class?
The generate function would then be a method on that class?
21f0b41 to
fbd6d4e
Compare
fbd6d4e to
709d608
Compare
d26e0c3 to
1379fa9
Compare
|
On a second look, the limb subdivision changes are definitely undercooked. And, given the usefulness of the material coordinates to the other projects we are working on, I decided to move the other limb subdivision changes to a separate branch, and just keep the following changes:
|
2 participants
Whole-body scaffold is in need of a material coordinates field. This pull request includes the following changes to the wholebody2 scaffold: