Skip to content

Conversation

@greschd
Copy link
Member

@greschd greschd commented Jan 31, 2023

TODO:

@greschd greschd linked an issue Jan 31, 2023 that may be closed by this pull request
Name of the Edge Set.
edge_set_type :
Determines how the Edge Set is defined. Can be either :attr:`.EdgeSetType.BY_NODES`
or :attr:`.EdgeSetType.BY_REFERENCE`.
Copy link
Contributor

@roosre roosre Feb 7, 2023

Choose a reason for hiding this comment

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

The same as in ACP. BY_REFERENCE tells the customer nothing.
What about or :attr:.EdgeSetType.BY_REFERENCE which means by :class:ElementSet.
Or you just read the doc of the other properties. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved by (almost) copying the GUI tooltips 👍

def __init__(
self,
name: str = "EdgeSet",
edge_set_type: EdgeSetType = EdgeSetType.BY_NODES,
Copy link
Contributor

Choose a reason for hiding this comment

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

Default in ACP is by reference, and I think that also makes more sense.

Element Set whose boundary the Edge Set follows.
Only applies when ``edge_set_type`` is :attr:`.EdgeSetType.BY_REFERENCE`.
limit_angle :
Maximum angle above which the remaining Element Set boundary is no
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we document here that -1 stand for no or suppressed limit angle to walk around the entire Element Set?

And I guess the angle is in degree? Please document.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm tempted to add a conversion from None to -1, to avoid the magic number.

Copy link
Contributor

@roosre roosre Feb 7, 2023

Choose a reason for hiding this comment

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

In this case, there is typically a bool to control whether the property is active (can be set or not)
image

So we would have to add another parameter: trim edge set. If true, the user can set the limit angle.
What do you think? Can also be addressed later. Or we implement it in pyACP and apply the change to ACP kernel later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, in that case I think we can do it later.

If we do it just on the PyACP side, there's no way to recover the previous value when it's turned off and on again.

Copy link
Member Author

@greschd greschd Feb 9, 2023

Choose a reason for hiding this comment

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

There's a sneaky way we could implement this.. by just flipping the sign (AFAICT, all negative values disable cropping). But I think that logic is a bit too potentially confusing. In any case, we can always add a nicer way to handle this later.

In any case, I now documented to use -1. for turning it off.

@greschd greschd requested a review from roosre February 9, 2023 13:06
@greschd greschd enabled auto-merge (squash) February 10, 2023 14:21
@greschd greschd merged commit be89438 into main Feb 10, 2023
@greschd greschd deleted the feat/edge_set branch February 10, 2023 14:26
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.

Implement EdgeSet class

4 participants