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

Colon #43

Closed
wants to merge 15 commits into from
Closed

Colon #43

wants to merge 15 commits into from

Conversation

mlin865
Copy link
Contributor

@mlin865 mlin865 commented Mar 4, 2019

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.

In the main: looking good. Mostly need to remove some code before bringing in - don't want two tubemesh files, want all scaffolds using the new tubemesh generation, remove the old.
More comments on specific lines.

@@ -62,7 +62,7 @@ def checkOptions(options):
options[key] = 1
if (options['Number of elements around'] < 2) :
options['Number of elements around'] = 2
if (options['Tube type'] < 1 or options['Tube type'] > 3 ) :
if (options['Tube type'] < 1 or options['Tube type'] > 5 ) :
Copy link
Member

Choose a reason for hiding this comment

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

It is more convenient it <1 --> 1 and > 5 --> 5.

return [
'Default',
'Human 1',
'Section 1']
Copy link
Member

Choose a reason for hiding this comment

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

Can 'Section 1' be given a more useful name? Is it temporary?

def getDefaultOptions(parameterSetName='Default'):
options = {
'Number of elements around': 15,
'Number of elements along haustra': 4,
Copy link
Member

Choose a reason for hiding this comment

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

Use singular: haustrum.

'Interhaustra fold factor']:
if options[key] > 1.0:
options[key] = 1.0
if (options['Tube type'] < 1 or options['Tube type'] > 3 ) :
Copy link
Member

Choose a reason for hiding this comment

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

As above >3 --> 3

[rotAxis[1]*rotAxis[0]*C + rotAxis[2]*sinTheta, rotAxis[1]*rotAxis[1]*C + cosTheta, rotAxis[1]*rotAxis[2]*C - rotAxis[0]*sinTheta],
[rotAxis[2]*rotAxis[0]*C - rotAxis[1]*sinTheta, rotAxis[2]*rotAxis[1]*C + rotAxis[0]*sinTheta, rotAxis[2]*rotAxis[2]*C + cosTheta]])

return rotMatrix
Copy link
Member

Choose a reason for hiding this comment

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

It's standard to finish on newline i.e. press enter on last line. Otherwise added lines on the next commit appear to modify the last line.

elementsCountAlong = elementsCountAlongUnit*unitsCountAlong

# Sample central line to get same number of elements as elementsCountAlong
sx, sd1, se, sxi, _ = sampleCubicHermiteCurves(cx, cd1, elementsCountAlong)
Copy link
Member

Choose a reason for hiding this comment

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

As this is not using arcLengthDerivatives=true, it won't match the length returned for the colon, so haustra will not finish exactly at the end.

using a unit profile.
Created on 18 Feb, 2019

@author: Mabelle Lin
Copy link
Member

Choose a reason for hiding this comment

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

We're now removing all @author throughout the code (including my own); the record of who did what is in git.

@@ -0,0 +1,356 @@
'''
Copy link
Member

Choose a reason for hiding this comment

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

Merge this new implementation into tubemesh.py, replacing old one.
The original centrallinetube1 scaffold can be migrated or discarded. I'm guessing what it brought to the table was ellipse profiles with equal element spacing, so not sure how/whether to support?
What you have is likely able to get into the code for now, so following are largely future discussions:
Soon we will need variable wall thickness around and along units/segments, plus in each layer, and also controllable side axis or axes to produce twists. Here I'm thinking your approach of having unit side axes and having all sizing information in the unit/segments could work well, then e.g. ellipsoidal sections can be passed in.
Furthermore, eventually the nodes should be numbered from one end to the other, and elements too. However, xi1 can remain around, xi2 along and xi3 through the wall.
Same function should also support generating 2D meshes in future.


return xList, dx_ds1List, dx_ds2List, dx_ds3List

def rotationMatrixAboutAxis(rotAxis, theta):
Copy link
Member

Choose a reason for hiding this comment

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

See earlier comment.
Whenever you find you've put the same function in 2 files you should remove it to a common file.

from opencmiss.zinc.field import Field
from opencmiss.zinc.node import Node

def generatetubemesh(region,
Copy link
Member

Choose a reason for hiding this comment

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

Note I haven't studied the implementation of this function, just the interface (which is fine for now).
I'm happy to bring it in as-is now, but will look at it in more detail when additional complications for variable wall thickness are added.

@mlin865
Copy link
Contributor Author

mlin865 commented Mar 8, 2019

Cancel this pull request in view of revisions made on a new branch with a new pull request.

@mlin865 mlin865 closed this Mar 8, 2019
rchristie added a commit that referenced this pull request Mar 11, 2019
Revisions based on comments from Colon Pull request #43
@mlin865 mlin865 deleted the noDeformation branch March 13, 2019 22:27
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