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

Base_Engine: Hash to leverage the new aggregating GeometryHash method for geometry types #3255

Closed
alelom opened this issue Jan 17, 2024 · 3 comments · Fixed by #3268
Closed
Assignees
Labels
type:feature New capability or enhancement

Comments

@alelom
Copy link
Member

alelom commented Jan 17, 2024

Description:

This depends on #3251 .

Currently, the Hash method traverses all properties of geometric types to create their signature. This can be extremely slow, and can instead leverage the GeometryHash method(s). In particular, when we will have implemented the aggregating GeometryHash method that returns a single number (#3251), we can use it instead of this property traversal for geometric types in the main Hash method. We can expose this as the default behaviour, but also have an option in the ComparisonConfig to toggle the old property traversal behaviour for Geometric types, if required for some reason. Further, we can expose the GeometryHash optional arguments such as the translation tolerance as properties of the ComparisonConfig class.

This will greatly increase the speed (and accuracy) of the Hash method for geometric types. Further, we will be able to always use the main Hash method for all use cases – including geometric types. This would make things more consistent and simpler to use.

Thoughts? @pawelbaran @al-fisher

@alelom alelom added the type:feature New capability or enhancement label Jan 17, 2024
@alelom alelom self-assigned this Jan 17, 2024
@pawelbaran
Copy link
Member

Looks like the great minds think alike! 😃 I had the same thought when I first learnt about GeometryHash - why aren't we calling it from the Hash method?! So yes, 100% support, especially that I do not think there is any precedent where we would persist the hashes at the moment - most users compare on the fly, i.e. if what they think is the same still comes back as the same, it is not a breaking change 👍

@alelom
Copy link
Member Author

alelom commented Jan 17, 2024

I had the same thought when I first learnt about GeometryHash - why aren't we calling it from the Hash method

Mainly just because we've written GeometryHash after Hash to solve a specific problem, but it was always in consideration (at least for me) to have it called from there.

if what they think is the same still comes back as the same, it is not a breaking change

I agree we shouldn't consider it a breaking change. However, it could be considered breaking of workflows were the hash has been stored as text somewhere as a "reference signature" of a set of objects. I don't think the usage is yet wide enough to warrant a worry about such cases.

@al-fisher
Copy link
Member

Yes - 100% 😸 Love this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New capability or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants