Bladder scaffold #62
Bladder scaffold #62
Conversation
There was a problem hiding this comment.
In general it's readable and looking good; I do have some comments in the code but first follow these instructions:
Due to some other developments, you'll need to get the latest latest opencmiss.utils from:
https://github.com/OpenCMISS-Bindings/opencmiss.utils, which should be installed (need to do the pip uninstall opencmiss.utils, git install -e . from the source - please ask me to help!).
Many of the functions that were in zinc_utils are now imported from opencmiss.utils.
You'll need to merge latest changes from scaffoldmaker from the prime repo on ABI-Software and fix the place where you currently use zinc_utils (I've made an in-source comment there.)
One of the recent changes is that source has moved to the src folder and there is a folder tests. Can you please make a test for the bladder scaffold in the style of the heart or one of the colon test files.
All the remaining comments in the code are to do with style and naming and should be quick to fix.
It's important that we get the bladder scaffold in now so I'm not yet commenting on the actual scaffold generation code, which I'll try out when merged and we can do further development on after then.
| options = { | ||
| 'Ureter': copy.deepcopy(ostiumOption), | ||
| 'Number of elements up 1': 8, | ||
| 'Number of elements up 2': 16, | ||
| 'Number of elements around': 16, # should be even | ||
| 'Number of elements through wall': 1, | ||
| 'Height': 3.0, | ||
| 'Major diameter': 2.5, | ||
| 'Minor diameter': 1.5, | ||
| 'Urethra radius': 0.35, | ||
| 'Bladder wall thickness': 0.05, | ||
| 'Number of elements around ostium': 8, | ||
| 'Ostium position around': 0.55, | ||
| 'Ostium position up': 0.65, | ||
| 'Number of elements radially on annulus': 2, | ||
| 'Use cross derivatives': False | ||
| } |
There was a problem hiding this comment.
These lines should each individually change the options only where they differ from the defaults, and to guarantee that all options are defined in all cases (you'll see the 'Refine...' options are lost for rat.)
I.e.
options['Number of elements around'] = 16
options['Height'] = 3.0
etc.
| 'Minor diameter': 3.0, | ||
| 'Urethra radius': 0.5, | ||
| 'Bladder wall thickness': 0.05, | ||
| 'Number of elements around ostium': 8,# implemented for 8 |
There was a problem hiding this comment.
Scaffoldmaker style is to put the 'Number' options together at the top of the list, 'Refine' at the bottom.
List each section in alphabetic order (I make an exception only where list so big that related options need to be adjacent to each other).
| 'Ostium position around', | ||
| 'Ostium position up', | ||
| 'Ureter', | ||
| 'Number of elements radially on annulus', |
There was a problem hiding this comment.
Scaffoldmaker style is to put the 'Number' options together at the top of the list, 'Refine' at the bottom.
List each section in alphabetic order.
There was a problem hiding this comment.
Some items not completed from previous review - see unresolved conversations.
I've added or copied a couple of new issues.
All very minor now - should take a couple of minutes.
Also need to add a test case making a simple bladder and confirming it is broadly the right size, has the right numbers of elements etc. See heart and colon tests.
| @@ -128,40 +128,34 @@ def getDefaultOptions(cls, parameterSetName='Default'): | |||
|
|
|||
| if 'Rat' in parameterSetName: | |||
| options = { | |||
There was a problem hiding this comment.
This hasn't been done:
These lines should each individually change the options only where they differ from the defaults, and to guarantee that all options are defined in all cases (you'll see the 'Refine...' options are lost for rat.)
I.e.
options['Number of elements around'] = 16
options['Height'] = 3.0
etc.
| cosRadiansUp = math.cos(radiansUp) | ||
| sinRadiansUp = math.sin(radiansUp) | ||
| MajorDiameter = majorDiameter * sinRadiansUp - n3 * bladderWallThickness | ||
| MinorDiameter = minorDiameter * sinRadiansUp - n3 * bladderWallThickness | ||
| MajorRadius = 0.5 * majorDiameter * sinRadiansUp - n3 * bladderWallThickness |
There was a problem hiding this comment.
Follow the common name convention to start variables with lower case letters, start Types with Upper case letters.
|
|
||
| # create elements | ||
| # # create elements |
There was a problem hiding this comment.
Extra comment character not needed.
Code for bladder scaffold is committed and pushed. The scaffold currently contains two parameter sets: rat and cat.