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

Clean up and test computation of normals in MeshData #2444

Merged
merged 5 commits into from
Jan 18, 2023

Conversation

asnt
Copy link
Member

@asnt asnt commented Jan 15, 2023

Supersedes #2098.

Summary:

  • Test computation of face normals.
  • Extract computation of face normals and vertex normals out of MeshData for clarity.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let me know if you want to do any more work on this besides what I describe below.

The only thing I'd like is: could you switch your assert statements in the meshdata.py code to if statements with regular ValueError exceptions? It is considered by some linting/complexity/security tools to be bad practice to use assert statements in non-testing code. Let me know what you think.

@asnt
Copy link
Member Author

asnt commented Jan 17, 2023

Looks good to me. Let me know if you want to do any more work on this besides what I describe below.

Thanks @djhoese for the review. That would be all from my side for this PR.

The only thing I'd like is: could you switch your assert statements in the meshdata.py code to if statements with regular ValueError exceptions? It is considered by some linting/complexity/security tools to be bad practice to use assert statements in non-testing code. Let me know what you think.

Agreed. Change applied.

I also changed the relevant raise Exception to raise ValueError.

Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

Love it! Thank you!

@djhoese djhoese merged commit 7ab945f into vispy:main Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants