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

Path1/sidederivatives #93

Merged
merged 10 commits into from
Sep 24, 2020

Conversation

elias-soltani
Copy link
Contributor

I would like to add some features to 1D path, please.

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.

Some small changes then it's in.

src/scaffoldmaker/meshtypes/meshtype_1d_path1.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_1d_path1.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_1d_path1.py Outdated Show resolved Hide resolved
src/scaffoldmaker/meshtypes/meshtype_1d_path1.py Outdated Show resolved Hide resolved
("Smooth d1 harmonic", lambda region, options: cls.smoothPath(region, options, DerivativeScalingMode.HARMONIC_MEAN)),
("Make D2 normal", lambda region, options: cls.makeD2Normal(region, options)),
("Make D3 normal", lambda region, options: cls.makeD3Normal(region, options))
]


def extractPathParametersFromRegion(region, groupName=None):
Copy link
Member

Choose a reason for hiding this comment

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

Pass in list of parameters required as list of node value labels e.g. valueLabels=[Node.VALUE_LABEL_VALUE, Node.VALUE_LABEL_D_DS1]. I think best to be a compulsory second argument.
Update callers to ensure still works. Your functions should only request the parameters they need.
Return sequence of lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not understand what you mean here. Do you mean like this?
("Make D2 normal", lambda region, options: cls.makeD2Normal(region, nodeValueLabels, options))
If yes, for D2 I need [Node.VALUE_LABEL_D_DS2] which right now is passed inside the function. Can you please tell me what do you mean?

Copy link
Member

@rchristie rchristie Sep 22, 2020

Choose a reason for hiding this comment

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

I meant only this function signature:

def extractPathParametersFromRegion(region, valueLabels, groupName=None)

No change to makeD2Normal arguments, only changes would be inside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for elaboration. I will make the change.

src/scaffoldmaker/utils/vector.py Outdated Show resolved Hide resolved
src/scaffoldmaker/utils/vector.py Outdated Show resolved Hide resolved
@elias-soltani
Copy link
Contributor Author

Thanks for reviewing the code.

Comment on lines 232 to 248
return cx, cd1, cd2, cd12, cd3, cd13
result = ()
for label in valueLabels:
if label == Node.VALUE_LABEL_VALUE:
result += (cx,)
if label == Node.VALUE_LABEL_D_DS1:
result += (cd1,)
if label == Node.VALUE_LABEL_D_DS2:
result += (cd2,)
if label == Node.VALUE_LABEL_D_DS3:
result += (cd3,)
if label == Node.VALUE_LABEL_D2_DS1DS2:
result += (cd12,)
if label == Node.VALUE_LABEL_D2_DS1DS3:
result += (cd13,)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the solution I'm looking for; should instead build the result directly and loop over the number of valueLabels and call getNodeParameters only for those requested in the code earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the code so it only gets the values specified by valueLabels. What we return depends on the values specified by the valueLabels, I do not know if there is another way to do this. Can you please tell me what else can be done?

Copy link
Member

@rchristie rchristie Sep 23, 2020

Choose a reason for hiding this comment

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

Combine with the evaluation loop:

valueLabelsCount = len(valueLabels)
returnValues = ( [] for i in range(valueLabelsCount) )
while node.isValid():
        cache.setNode(node)
        for i in range(valueLabelsCount):
            result, values  = coordinates.getNodeParameters(cache, -1, valueLabels[i], 1, componentsCount)
            for c in range(componentsCount, 3):
                values.append(0.0)
            returnValues[i].append(values)
        node = nodeIter.next()
return returnValues

Do check my logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brilliant! Thanks.

src/scaffoldmaker/utils/vector.py Outdated Show resolved Hide resolved
@rchristie rchristie merged commit 80afe7a into ABI-Software:master Sep 24, 2020
@elias-soltani elias-soltani deleted the path1/sidederivatives branch September 24, 2020 22:12
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