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

Use a looser tolerance on pitch for HexGrid roughly equal check #2058

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

drewj-tp
Copy link
Contributor

What is the change?

Use math.isclose to compare pitches of two hex grids rather than strict equality.

Why is the change being made?

We don't need a super strict check to make sure the grids are roughly equal. The ideal of the method is to check that one index location in one grid would be in approximately the same spatial location in the other grid. Otherwise we can't accurately rotate the position within the grid.

This problem came up in some shuffling work where two assemblies had ever so slightly different pitches, on the order of hundreths of a percent. The strict other.pitch == self.pitch failed even though, for the purposes of this method, the two grids are "roughly equal"


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@drewj-tp drewj-tp self-assigned this Jan 20, 2025
@mgjarrett
Copy link
Contributor

The default tolerance for math.isclose is 1e-9; I think we want this to be a little looser (like 1e-3 or 1e-4) to allow for two assemblies that have slightly different pin pitches but otherwise have the same number of pins.

@drewj-tp
Copy link
Contributor Author

The default tolerance for math.isclose is 1e-9; I think we want this to be a little looser (like 1e-3 or 1e-4) to allow for two assemblies that have slightly different pin pitches but otherwise have the same number of pins.

Not sure how that disappeared but I did write out a rel_tol=1e-4 locally...

Let me re-add that and add a test accordingly

@drewj-tp drewj-tp marked this pull request as draft January 20, 2025 17:55
We don't need a super strict check to make sure the grids are roughly
equal. The ideal of the method is to check that one index location in
one grid would be in approximately the same spatial location in the
other grid. Otherwise we can't accurately rotate the position within the
grid.

This problem came up in some shuffling work where two assemblies had
ever so slightly different pitches, on the order of hundreths of a
percent. The strict `other.pitch == self.pitch` failed even though, for
the purposes of this method, the two grids are "roughly equal"
@drewj-tp drewj-tp force-pushed the drewj/looser-hex-grid-rough-equal branch from ce1cfa8 to 121d2c2 Compare January 20, 2025 21:15
Copy link
Contributor

@mgjarrett mgjarrett left a comment

Choose a reason for hiding this comment

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

Looks good, just need to add release notes

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.

2 participants