Skip to content

Conversation

@Sam-Bouten
Copy link
Contributor

Included queries (_where and _where_predicate) for elements of VolMesh (halfface.py),
that up until now were placeholders or had no interface declared.
Consistent with the Mesh datastructure (halfedge.py) query functions.

resolves issue #906.

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

  • 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)

Include queries (where and predicates) for composing elements of VolMesh. Consistent with Mesh datastructure query functions.
Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

this seems very testable. please add some :)

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

generally looking good, but could you add a few test?

@gonzalocasas
Copy link
Member

@brgcode could you pls take a look at this again and confirm if it's good to go?

@tomvanmele
Copy link
Member

@brgcode could you pls take a look at this again and confirm if it's good to go?

it never was, but i seem to have forgotten to submit my review...

for u in cell[c]:
for v in cell[c][u]:
faces.append(cell[c][u][v])
faces.append(cell[c][u][v])
Copy link
Member

Choose a reason for hiding this comment

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

why is this loop removed? this cannot possibly be expected to yield the same result...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrongly left out one loop when changing halfface data init, reverted this to main now.

def index_vertex(self):
"""Returns a dictionary that maps the indices of a vertex list to
keys in the vertex dictionary.
keys in a vertex dictionary.
Copy link
Member

Choose a reason for hiding this comment

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

should be "the", since it refers to "the" vertex dictionary used for internal storage, not "a" random vertex dictionary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, shall we update the same in docstring of halfedge key_index() method.

Comment on lines 804 to 807
key: hashable
The next vertex that matches the condition.
2-tuple
The next vertex and its attributes, if ``data=True``.
Copy link
Member

Choose a reason for hiding this comment

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

same comment

@tomvanmele
Copy link
Member

@Sam-Bouten can you update your main and merge it into the PR again? it will solve the failed docs workflow...

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

can you remove the double backticks from the docstrings, please?
reference to a parameter name is

`param`

and True and False are without anything...

@tomvanmele tomvanmele merged commit 011c777 into compas-dev:main Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants