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

Multiple annotation groups #80

Merged
merged 3 commits into from
Jul 22, 2020
Merged

Multiple annotation groups #80

merged 3 commits into from
Jul 22, 2020

Conversation

mlin865
Copy link
Contributor

@mlin865 mlin865 commented Jul 20, 2020

No description provided.

@rchristie
Copy link
Member

Can you walk me through this. I'm unsure why groups are being looked up by name when object can be used more efficiently? Also wondering about the ramifications of having a separate list for along, around and through the wall. What if there were different groups through the wall for some elements around?
Even if all is well there are some pythonic tricks that will make the code cleaner e.g.
if name in names:
(where name is a str and names is a list of str).

Comment on lines 1859 to 1871
for annotationGroup in annotationGroups:
if annotationArrayAround[e1] == annotationGroup._name or \
annotationArrayAlong[0] == annotationGroup._name or \
annotationArrayThroughWall[e3] == annotationGroup._name:
meshGroup = annotationGroup.getMeshGroup(mesh)
meshGroup.addElement(element)
for annotationAround in annotationArrayAround[e1]:
if annotationAround == annotationGroup._name:
meshGroup = annotationGroup.getMeshGroup(mesh)
meshGroup.addElement(element)
for annotationAlong in annotationArrayAlong[0]:
if annotationAlong == annotationGroup._name:
meshGroup = annotationGroup.getMeshGroup(mesh)
meshGroup.addElement(element)
for annotationThroughWall in annotationArrayThroughWall[e3]:
if annotationThroughWall == annotationGroup._name:
meshGroup = annotationGroup.getMeshGroup(mesh)
meshGroup.addElement(element)
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't directly use members prefixed with an underscore e.g. annotationGroup._name as this is the convention for 'private' (but nothing is truly private in python).
Would at a minimum use a method to get name and can replace lines 1860-1861 with (I think this works, but actually suggest going further below):

                        if annotationGroup.getName() in annotationArrayAround[e1]:

(Incidentally is it correct that the index is [0] in line 1864; maybe add a dummy e2 set to 0?)
If annotationArrayAround[e1] was a list of AnnotationGroup can replace lines 1860 to 1863 with:

                        if annotationGroup in annotationGroupsAround[e1]:
                            meshGroup = annotationGroup.getMeshGroup(mesh)
                            meshGroup.addElement(element)

Another alternative is to just put the lists from each array into a common lists:

annotationGroups = annotationGroupsAround[e1] + annotationGroupsAlong[e2] + annotationGroupsThroughWall[e3]
for annotationGroup in annotationGroups:
    meshGroup = annotationGroup.getMeshGroup(mesh)
    meshGroup.addElement(element)

If the same group appears twice the second call to addElement returns a benign error. See:
http://opencmiss.org/documentation/apidoc/zinc/latest/classOpenCMISS_1_1Zinc_1_1MeshGroup.html#ab0fa0e1b8ba89a3543c75c4c4c70f64d

@mlin865
Copy link
Contributor Author

mlin865 commented Jul 21, 2020

Thanks for your suggestions, Richard. The code looks alot more elegant now with the revision. Please review the changes.

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.

Nice.

@rchristie rchristie merged commit 5740b58 into ABI-Software:master Jul 22, 2020
@mlin865 mlin865 deleted the cecum branch July 22, 2020 00:38
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