Add some more str function to geometry classes for pretty print #1289

yck011522 · 2024-01-31T16:13:58Z

Partially Fixes #1287

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)

The truth value of an array with more than one element is ambiguous.

…d Polyhedron classes

gonzalocasas

I added a few comments but generally looking good!

src/compas/geometry/frame.py

src/compas/geometry/plane.py

src/compas/geometry/pointcloud.py

gonzalocasas · 2024-02-01T08:39:28Z

src/compas/geometry/pointcloud.py

-        points = box.points
-        x, y, z = zip(*points)
-        xmin, xmax = min(x), max(x)
-        ymin, ymax = min(y), max(y)
-        zmin, zmax = min(z), max(z)
+        xmin, xmax = box.xmin, box.xmax
+        ymin, ymax = box.ymin, box.ymax
+        zmin, zmax = box.zmin, box.zmax


This change is not related to the str stuff, is it?

The change is there because I couldn't actually run tests without fixing the broken functions.

gonzalocasas · 2024-02-01T08:39:42Z

src/compas/geometry/polygon.py

@@ -103,6 +103,9 @@ def __init__(self, points, name=None):
    def __repr__(self):
        return "{0}(points={1!r})".format(type(self).__name__, self.points)

+    def __str__(self):
+        return "{0}(points={1})".format(type(self).__name__, [point.__str__() for point in self.points])


switch to str() as above

src/compas/geometry/polyhedron.py

tomvanmele · 2024-02-01T12:26:00Z

@yck011522 please keep the PR focused on the implementation of __str__. adding other stuff will just drag out the review process...

yck011522 · 2024-02-02T04:38:39Z

@yck011522 please keep the PR focused on the implementation of __str__. adding other stuff will just drag out the review process...

@tomvanmele The associated changes are there only because those functions were broken to the point that prevented me to test out the new __str__ functios.

gonzalocasas

sorry, this slipped through the cracks... it lgtm!

gonzalocasas · 2024-07-02T11:46:19Z

@yck011522 could you fix the conflict so that we can merge?

gonzalocasas

Besides the changelog fix it lgtm

gonzalocasas · 2024-07-03T13:22:26Z

CHANGELOG.md

@@ -268,6 +268,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Changed `compas.scene.VolMesh`'s `show_vertices`, `show_edges`, `show_faces`, `show_cells` to optionally accept a sequence of keys.
 * Fixed missing implementation of `Sphere.base`.
 * Fixed bug in `intersection_sphere_sphere`.
+* Changed the `__str__` of `compas.geometry.Frame`, `compas.geometry.Plane`, `compas.geometry.Polygon`, `compas.geometry.Polyhedron`, `compas.geometry.Quaternion` to use a limited number of decimals (determined by `Tolerance.PRECISION`). Note: `__repr__` will instead maintain full precision.


Could you update this? Because after the merge it's way down in the file, changing an already released version

Moved lines up

codecov · 2024-07-10T02:03:12Z

Codecov Report

Attention: Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.19%. Comparing base (b23bf7b) to head (7d65bbb).

Files	Patch %	Lines
src/compas/colors/color.py	75.00%	1 Missing ⚠️
src/compas/geometry/frame.py	50.00%	1 Missing ⚠️
src/compas/geometry/plane.py	50.00%	1 Missing ⚠️
src/compas/geometry/pointcloud.py	80.00%	1 Missing ⚠️
src/compas/geometry/polygon.py	50.00%	1 Missing ⚠️
src/compas/geometry/polyhedron.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
+ Coverage   60.11%   60.19%   +0.07%     
==========================================
  Files         207      207              
  Lines       22234    22248      +14     
==========================================
+ Hits        13366    13392      +26     
+ Misses       8868     8856      -12

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

yck011522 · 2024-07-10T02:07:59Z

@tomvanmele @gonzalocasas
I notice that the new check called codecov is not passing. The checker seems to want every line of code being included in test. How should I handle this? Should I make the compliant? It seems like some of the tests will be excessive if every line needs to be in CI.

Because newer float format behavior is different in different python versions

there seems to be a problem with TOL.format_numbers not passing test

yck011522 · 2024-07-10T03:25:07Z

I was trying to add some more tests to cover the str and repr functions. However I realized that the underlying mechanism of formatting the numbers in Point is based on TOL.format_number and its behavior is unstable across different environments.

So I have decided to open another PR to investigate the problem and this PR will not have those tests included.

gonzalocasas · 2024-07-10T05:16:01Z

@tomvanmele @gonzalocasas
I notice that the new check called codecov is not passing. The checker seems to want every line of code being included in test. How should I handle this? Should I make the compliant? It seems like some of the tests will be excessive if every line needs to be in CI.

Having coverage metrics is good, but we don't intend to be obsessive about it

tomvanmele · 2024-07-10T08:27:09Z

@yck011522 the test is just feedback about coverage. it will not block the PR from being merged...

yck011522 added 6 commits January 31, 2024 23:45

Add __str__ to frame and plane

4dd2def

Fixes broken Pointcloud.from_box

073a2ab

Fixes ValueError:

d7c3dd5

The truth value of an array with more than one element is ambiguous.

Update __repr__ and __str__ methods in Color, Pointcloud, Polygon, an…

e4d965e

…d Polyhedron classes

black and lint

cef3f8e

Add __str__ method to Quaternion class

5b3a267

gonzalocasas reviewed Feb 1, 2024

View reviewed changes

yck011522 added 3 commits February 2, 2024 12:52

Use str() instead of calling dunder within dunder

d2b2f65

changelog

03f79a9

pointcloud

ac0e8ce

tomvanmele marked this pull request as ready for review May 31, 2024 11:43

tomvanmele requested review from gonzalocasas, Licini and chenkasirer May 31, 2024 11:43

gonzalocasas approved these changes Jul 2, 2024

View reviewed changes

Merge branch 'main' into yck011522/issue1287

bc0c649

gonzalocasas approved these changes Jul 3, 2024

View reviewed changes

yck011522 added 5 commits July 10, 2024 03:43

Update CHANGELOG.md

25d1d70

Moved lines up

Update CHANGELOG.md

af7720c

Merge branch 'main' into yck011522/issue1287

4d325f6

black

f5fa387

Fix Quaternion comparison test in test_quaternion_other_methods

13e77de

yck011522 added 3 commits July 10, 2024 10:14

test pointcloud

7c52617

bug

2f3c0c2

tests for str and repr

bf90aba

yck011522 added 3 commits July 10, 2024 10:46

Test __str___ but not __repr__

3d25912

Because newer float format behavior is different in different python versions

remove __str__ tests,

31224fa

there seems to be a problem with TOL.format_numbers not passing test

ipy special

7d65bbb

yck011522 merged commit f4cf9cb into main Jul 10, 2024
17 checks passed

yck011522 deleted the yck011522/issue1287 branch July 11, 2024 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some more str function to geometry classes for pretty print #1289

Add some more str function to geometry classes for pretty print #1289

yck011522 commented Jan 31, 2024

gonzalocasas left a comment

gonzalocasas Feb 1, 2024

yck011522 Feb 2, 2024

gonzalocasas Feb 1, 2024

tomvanmele commented Feb 1, 2024

yck011522 commented Feb 2, 2024

gonzalocasas left a comment

gonzalocasas commented Jul 2, 2024

gonzalocasas left a comment

gonzalocasas Jul 3, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

yck011522 commented Jul 10, 2024

yck011522 commented Jul 10, 2024

gonzalocasas commented Jul 10, 2024

tomvanmele commented Jul 10, 2024

Add some more __str__ function to geometry classes for pretty print #1289

Add some more __str__ function to geometry classes for pretty print #1289

Conversation

yck011522 commented Jan 31, 2024

What type of change is this?

Checklist

gonzalocasas left a comment

Choose a reason for hiding this comment

gonzalocasas Feb 1, 2024

Choose a reason for hiding this comment

yck011522 Feb 2, 2024

Choose a reason for hiding this comment

gonzalocasas Feb 1, 2024

Choose a reason for hiding this comment

tomvanmele commented Feb 1, 2024

yck011522 commented Feb 2, 2024

gonzalocasas left a comment

Choose a reason for hiding this comment

gonzalocasas commented Jul 2, 2024

gonzalocasas left a comment

Choose a reason for hiding this comment

gonzalocasas Jul 3, 2024

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

yck011522 commented Jul 10, 2024

yck011522 commented Jul 10, 2024

gonzalocasas commented Jul 10, 2024

tomvanmele commented Jul 10, 2024

Add some more str function to geometry classes for pretty print #1289

Add some more str function to geometry classes for pretty print #1289

codecov bot commented Jul 10, 2024 •

edited

Loading