-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Additional Nodes functionality and Node robustness #22
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of good work here :)
Take a look at my comments when you have time and we can discuss how\if we should implement my suggested edits prior to merge
src/ada/core/utils.py
Outdated
elem.nodes.pop(node_index) | ||
elem.nodes.insert(node_index, new_node) | ||
elem._shape = None | ||
logging.debug(f"{old_node} exchanged with {new_node} --> {elem}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the ref on the old node referring to the element it was just deleted from?
Consider adding something like this at the end of each element eval
old_node.refs.pop(old_node.refs.index(elem))
src/ada/core/utils.py
Outdated
try: | ||
ref_index = elem.nodes.index(old) | ||
node_index = elem.nodes.index(old_node) | ||
except ValueError: | ||
logging.error(f"Could not find {old_node} in {elem}") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is logging error and return the right mechanism here? Obviously if a element does not contain a node which thinks it is a member of the element then something is probably very wrong.
Therefore I suggest a raise ValueError might be a better fit here stopping any further execution since this error should in principle never happen?
If relevant a pull request should include
Remember to perform linting before making a pull request