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

Update dependencies to swap in cytriangle #439

Merged
merged 4 commits into from
Jun 20, 2024
Merged

Conversation

connorferster
Copy link
Collaborator

@connorferster connorferster commented Jun 11, 2024

Pull Request Check List

  • Added tests for changed code.
  • Updated documentation for changed code.
  • Run the Nox test suite to check for errors and warnings.
  • Check benchmarks triangle vs cytriangle

@m-clare
Copy link

m-clare commented Jun 11, 2024

Patch release v1.0.1 adds macos-13 builds to PyPi.

I can add typing this weekend to appease mypy if necessary.

@robbievanleeuwen
Copy link
Owner

Thanks @m-clare, no need to add typing, I excluded triangle from mypy previously and am happy to exlude cytriangle which now fixes those workflows.

@connorferster I have a few weeks off work starting next week so am happy to update to update the docs and merge.

Also, we are now testing against macos-14 (i.e. arm64), yay 🚀 Note that rhino3dm has been updated to suit and the tests pass, however I have issues with installing recent versions of rhino3dm on my mac at home, see mcneel/rhino3dm#622. I think there is an issue with the universal wheels but not 100% sure? Anyway I am happy to merge this branch for now, if a user experiences this it's a pretty easy fix by manually downloading the appropriate rhino3dm wheel from pypi and installing into their virtual environment.

@robbievanleeuwen robbievanleeuwen added the enhancement New feature or request label Jun 13, 2024
@connorferster connorferster merged commit 0a41ea1 into master Jun 20, 2024
25 checks passed
@connorferster
Copy link
Collaborator Author

Hi @robbievanleeuwen,

I will merge this but then also ask: how do I check the benchmarks?

@robbievanleeuwen
Copy link
Owner

Thanks @connorferster, there's a bit of info here about the benchmarks. Basically just runs a set of tests and gives some time info as output. I can have a look at these tomorrow, will just checkout the code before and after the PR merge to see if there's any significant changes. I'll also quickly update the docs where it references the old triangle library - should be pretty quick with a little ctrl-f action!

@robbievanleeuwen
Copy link
Owner

Benchmarking

These results are in no way scientific (run on a linux virtual machine with background tasks going running on my PC):

Meshing

triangle appears to be a fair bit faster than cytriangle, but note the results are in microseconds so don't make a huge difference to the runtime of `sectionproperties. The difference between the two libraries appears to grow as mesh size grows so maybe something to investigate if you're interested in optimisation? Again, take these numbers with a grain of salt as I'm no expert at benchmarking...

Cytriangle

mesh_cytriangle

Triangle

mesh_triangle

Analysis

This is a benchmark of the whole sectionproperties workflow. The cytriangle runs are faster than the triangle runs, I can't explain this. So maybe we throw everything out the window 😆

Cytriangle

analysis_cytriangle

Triangle

analysis_triangle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants