update unify cycles #1413

romanarust · 2024-11-14T11:57:24Z

As noted in #1411 there is a problem with face_adjacency on meshes with arbitrary-sized faces. I've now exposed the parameters radius and nmax from compas.topology._face_adjacency to compas.topology.face_adjacency and further propagated them to unify_cycles and Mesh.unify_cycles.

What type of change is this?

Bug fix in a backwards-compatible manner.
New feature in a backwards-compatible manner.
Breaking change: bug fix or new feature that involve incompatible API changes.
Other (e.g. doc update, configuration, etc)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

codecov · 2024-11-14T11:59:56Z

Codecov Report

Attention: Patch coverage is 68.75000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.69%. Comparing base (f24872a) to head (fc9e736).
Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
src/compas/topology/orientation.py	66.66%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
+ Coverage   61.57%   61.69%   +0.11%     
==========================================
  Files         207      207              
  Lines       22281    22292      +11     
==========================================
+ Hits        13720    13753      +33     
+ Misses       8561     8539      -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tomvanmele · 2024-11-14T17:07:26Z

src/compas/datastructures/mesh/mesh.py

        """Unify the cycles of the mesh.

+        Parameters
+        ----------
+        root : str, optional


type of root is int

tomvanmele · 2024-11-14T17:07:59Z

src/compas/topology/orientation.py

@@ -166,6 +172,10 @@ def unify_cycles(vertices, faces, root=None):
        The faces of the mesh defined as lists of vertex indices.
    root : str, optional


tomvanmele · 2024-11-14T17:13:00Z

src/compas/topology/orientation.py

+    if nmax is not None:
+        if radius is not None:
+            return _face_adjacency(points, faces, nmax=nmax, radius=radius)
+        return _face_adjacency(points, faces, nmax=nmax)
+    if radius is not None:
+        return _face_adjacency(points, faces, radius=radius)


perhaps it would make sense to refactor _face_adjacency such that you can write here

if nmax or radius: return _face_adjacency(points, faces, nmax=nmax, radius=radius)

tomvanmele · 2024-11-14T17:14:35Z

btw, since the switch is no longer based on the number of faces, it should be easier to test using generated boundary cases...

romanarust · 2024-11-15T07:46:54Z

btw, since the switch is no longer based on the number of faces, it should be easier to test using generated boundary cases...

correct. do you have something in mind or should I leave the tests for now?

tomvanmele · 2024-11-15T09:01:42Z

was thinking that we could use the meshgrid with awkward proportions in x and y to generate inputs where radius and nmax provide very different results in different directions...

tomvanmele

LGTM

overlooked missing assertion

tomvanmele

everything looks good but the tests are a bit confusing.
should we not include an assertion of some sorts?
also, if you know when the tests are supposed to fail, should we then not add a @pytest.mark.xfail?

tomvanmele · 2024-12-10T21:55:43Z

tests/compas/topology/test_unify_cycles.py

+    vertices = test_data["vertices"]
+    faces = test_data["faces"]
+    unify_cycles(vertices, faces)
+    unify_cycles(vertices, faces, nmax=29, max_distance=22.4)  # anything below won't work


is it clear why that is the case?
or it is just something you noticed?

tomvanmele · 2024-12-10T21:57:44Z

tests/compas/topology/test_unify_cycles.py

+    unify_cycles(vertices, faces, nmax=29, max_distance=22.4)  # anything below won't work
+
+
+def test_face_adjacency():


why is this a face adjacency test and not also a test of unify cycles?

tomvanmele · 2024-12-10T22:03:13Z

@romanarust perhaps just a few things to sort out regarding the test(s)

romanarust · 2024-12-13T06:56:19Z

@romanarust perhaps just a few things to sort out regarding the test(s)

Thanks! I've added now some assert to the tests.

tomvanmele

LGTM

update unify cycles

9b21995

romanarust requested a review from tomvanmele November 14, 2024 11:57

romanarust added 2 commits November 14, 2024 13:29

skip ipy

9bb9e91

Merge branch 'main' into fix_unify_cyc

4d0da5e

tomvanmele reviewed Nov 14, 2024

View reviewed changes

updates

38a7049

romanavyzn and others added 6 commits November 15, 2024 15:52

adding meshgrid test

155147b

updates

77ee059

updates

44c7d8c

Update mesh.py

b02bde9

Update mesh.py

e33f06a

Update orientation.py

c13863b

tomvanmele previously approved these changes Dec 10, 2024

View reviewed changes

tomvanmele self-requested a review December 10, 2024 21:56

tomvanmele reviewed Dec 10, 2024

View reviewed changes

Update test_unify_cycles.py

fc9e736

tomvanmele approved these changes Dec 13, 2024

View reviewed changes

romanarust merged commit 347e5ec into main Dec 13, 2024
18 checks passed

romanarust deleted the fix_unify_cyc branch December 13, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update unify cycles #1413

update unify cycles #1413

romanarust commented Nov 14, 2024

codecov bot commented Nov 14, 2024 •

edited

Loading

tomvanmele Nov 14, 2024

tomvanmele Nov 14, 2024

tomvanmele Nov 14, 2024

tomvanmele commented Nov 14, 2024

romanarust commented Nov 15, 2024

tomvanmele commented Nov 15, 2024

tomvanmele left a comment

tomvanmele left a comment

tomvanmele Dec 10, 2024

tomvanmele Dec 10, 2024

tomvanmele commented Dec 10, 2024

romanarust commented Dec 13, 2024

tomvanmele left a comment

		@@ -166,6 +172,10 @@ def unify_cycles(vertices, faces, root=None):
		The faces of the mesh defined as lists of vertex indices.
		root : str, optional

		unify_cycles(vertices, faces, nmax=29, max_distance=22.4) # anything below won't work


		def test_face_adjacency():

update unify cycles #1413

update unify cycles #1413

Conversation

romanarust commented Nov 14, 2024

What type of change is this?

Checklist

codecov bot commented Nov 14, 2024 • edited Loading

Codecov Report

tomvanmele Nov 14, 2024

Choose a reason for hiding this comment

tomvanmele Nov 14, 2024

Choose a reason for hiding this comment

tomvanmele Nov 14, 2024

Choose a reason for hiding this comment

tomvanmele commented Nov 14, 2024

romanarust commented Nov 15, 2024

tomvanmele commented Nov 15, 2024

tomvanmele left a comment

Choose a reason for hiding this comment

tomvanmele left a comment

Choose a reason for hiding this comment

tomvanmele Dec 10, 2024

Choose a reason for hiding this comment

tomvanmele Dec 10, 2024

Choose a reason for hiding this comment

tomvanmele commented Dec 10, 2024

romanarust commented Dec 13, 2024

tomvanmele left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2024 •

edited

Loading