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

Add fusiform muscle #180

Merged
merged 7 commits into from
Jun 3, 2022

Conversation

elias-soltani
Copy link
Contributor

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.

Need to catch up to main as mesh generator/Scaffold Creator requires some new services from it (see comments in other PR review).
Make default number of elements along 10 -- the shape doesn't change significantly after this number and the elements are less stretched.
Can you please add muscle annotation terms (annotation/muscle_terms.py) and with each muscle type you support e.g. https://scicrunch.org/sawg/interlex/search?q=brachioradialis.
This is a case where you should store the parameter set name in the options["Base parameter set"] and use it later. See the brainstem scaffold for guidance, and the stellate which actually uses the values!
In generateBaseMesh define only the term equivalent to the parameter set name, putting all elements in that annotation group.
The tendon and muscle groups should be done differently (or be removed for now). Best is to follow what is done for the stomach and bladder/LUT, annotate the central path with groups representing each section of tendon/muscle, then generate the sections of cylinder to coincide with them i.e. the element lengths along are built to the tendon boundaries, rather than having even sized elements along the whole muscle and tendon groups made from some of these. All annotation groups need proper term names and ids from SciCrunch.

src/scaffoldmaker/meshtypes/meshtype_3d_musclefusiform1.py Outdated Show resolved Hide resolved
@@ -786,13 +789,25 @@ def createEllipsePerimeter(centre, majorAxis, minorAxis, elementsCountAround, he
unitMajorAxis = vector.normalise(majorAxis)
unitMinorAxis = vector.normalise(minorAxis)
useHeight = min(max(0.0, height), 2.0 * magMajorAxis)
totalRadians = geometry.getEllipseRadiansToX(magMajorAxis, 0.0, magMajorAxis - useHeight,
initialTheta=0.5 * math.pi * useHeight / magMajorAxis)
# check if it is a circle
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 not happy with having circle fixes here.
I'd prefer as a first fix that the ellipse functions do exact circle calculations if major and minor axes are equal (to a tolerance of total value).
I also request you take the time to make the general ellipse functions near exact precision as it's now pretty important, I can't spare the time, plus I think you're well capable. You'll have to do some brainstorming and come back to me with plans and algorithms, but I have some suggestions:

  1. make the ellipse perimeter function exact to a specified precision using iteration with series sums: have a think or look up algorithms such as one of these https://www.mathsisfun.com/geometry/ellipse-perimeter.html
  2. the function for advancing the angle around by an arc length around an ellipse accrues error. See about making it work to specified precision like the above. I note that many of the use cases could be changed to instead advance a fraction of N equal sized segments around the ellipse and restore precision on each quarter ellipse, or we may pass in the pre-calculated exact perimeter and current distance around to continuously improve the regular function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the code to be more precise.

@@ -64,19 +64,20 @@ def getEllipseArcLength(a, b, angle1Radians, angle2Radians):
else:
return -length

def updateEllipseAngleByArcLength(a, b, inAngleRadians, arcLength):
def updateEllipseAngleByArcLength(a, b, inAngleRadians, arcLength, tol=1.0E-4):
Copy link
Member

Choose a reason for hiding this comment

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

See above comment - we need to make this more precise.
As mentioned we could pass in exact perimeter and current distance around to restore precision.

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.

Looks great, like the use of scipy optimisation.
I would use an enum instead of passing (and comparing) a string for different modes of how the ellipse code is done. Can add this later. Look for other code with:
from enum import Enum

@rchristie rchristie merged commit f6c20c1 into ABI-Software:main Jun 3, 2022
@elias-soltani
Copy link
Contributor Author

Thanks for reviewing the code and good feedback as always.

@elias-soltani elias-soltani deleted the muscle/fusiform_muscle branch June 7, 2022 02:00
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