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

Diffing_Engine: Hash and Diff to leverage new GeometryHash #3268

Merged
merged 19 commits into from
Feb 1, 2024

Conversation

alelom
Copy link
Member

@alelom alelom commented Jan 29, 2024

NOTE: Depends on

BHoM/BHoM#1594

Issues addressed by this PR

Closes #3255
Closes #3269

The following was also implemented in this PR in order to be able to test properly the implementation of #3255.
Closes #3264

This PR makes the base Hash() use the GeometryHash() method by default (and consequently, the Diffing can also leverage it via the DiffWithHash() method). This can give a significant speed boost for an average of 33% less computation time, and a potential 99% for heavy geometry inputs. The average 33% speed improvement was measured via the DiffingTests_Prototypes DiffProfiling project and the results are as below:


Profiling results, running the DiffingProfiling project

The profiling considered an average use case with a mixture of geometrical and non-geometrical objects.

This profiling was repeated 20 times and gives an average value of about 33% less time.


In order to make this possible, the BH.Engine.Geometry.Query.HashArray() method now makes use of the BaseComparisonConfig options, bringing it on par to the base BH.Engine.Base.Query.Hash() method in terms of identification potential for Geometry objects. This is a requirement for any method that return an object signature (Hash). See #3269 for more information.

Test files

Same tests used in #3257, but calling BH.Engine.Base.Hash() instead of BH.Engine.Geometry.GeometryHash().

Please also run all tests in BHoM/DiffingTests_Prototypes#18.
They should all pass. Please note that the default value of the new GeometryHash option is set to true in the ComparisonConfig (see BHoM/BHoM#1594). The fact that all tests pass in the DiffingTests_Prototypes means that the new GeometryHash integration is fully compatible with the Hash() and Diffing() workflows and gives the same results, while also giving a performance boost.

Changelog

  • The base BH.Engine.Base.Query.Hash() method now leverages the BH.Engine.Geometry.Query.GeometryHash() method by default. This can give a significant speed boost, for an average of 33% less computation time, and a potential 99% for heavy geometry inputs.
  • The BH.Engine.Geometry.Query.HashArray() method now makes use of the BaseComparisonConfig options, bringing it on par to the base BH.Engine.Base.Query.Hash() method in terms of identification potential for Geometry objects.

@alelom alelom added the type:feature New capability or enhancement label Jan 29, 2024
@alelom alelom self-assigned this Jan 29, 2024
Faces get included in the HashArray only if:
1) they were not specified as a TypeException, and
2) a TypeException for Point was specified.

This is required because if both conditions are true, then at least "topological" information is included in the HashArray.
@alelom alelom force-pushed the Geometry_Engine-#3255-DiffUseGeometryHash branch from 7c10de6 to a8f4631 Compare January 31, 2024 14:09
@pawelbaran
Copy link
Member

@BHoMBot check compliance

Copy link

bhombot-ci bot commented Feb 1, 2024

@pawelbaran to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance

Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Great stuff @alelom, although not trivial to keep track of 🤯 I did my best to review it properly and took the following steps:

  • read the code changes - see comments, pardon my ignorance if any of them does not make sense, a few points are based more on intuition rather than in-depth debugging
  • ran GH test scripts I have built for this concept on previous occasions - all passed

BHoM_Engine/Query/Hash.cs Outdated Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Outdated Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Show resolved Hide resolved
Geometry_Engine/Query/HashArray.cs Show resolved Hide resolved
@pawelbaran
Copy link
Member

Test failures are gone after switching to the right branch in the diffing test repo, apologies and thanks for help @alelom!

alelom and others added 2 commits February 1, 2024 09:56
Co-authored-by: Pawel Baran <pawel.baran@burohappold.com>
Copy link
Member

@pawelbaran pawelbaran left a comment

Choose a reason for hiding this comment

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

Happy to approve following code review, testing and offline discussions with @alelom. Good stuff, thanks!

@alelom
Copy link
Member Author

alelom commented Feb 1, 2024

@BHoMBot check required

Copy link

bhombot-ci bot commented Feb 1, 2024

@alelom to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check copyright-compliance
@BHoMBot check dataset-compliance

Copy link

bhombot-ci bot commented Feb 1, 2024

@FraserGreenroyd to confirm, the following actions are now queued:

  • check copyright-compliance
  • check dataset-compliance

@FraserGreenroyd
Copy link
Contributor

@BHoMBot this is a DevOps instruction. I am requesting neutral checks on: unit-tests

Copy link

bhombot-ci bot commented Feb 1, 2024

@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.

Copy link

bhombot-ci bot commented Feb 1, 2024

@alelom just to let you know, I have provided a check-ready-to-merge result to this Pull Request as it was detected to be linked to other Pull Requests in a series. The comment which triggered this check came from @FraserGreenroyd on BHoM

@FraserGreenroyd FraserGreenroyd merged commit c74b262 into develop Feb 1, 2024
12 checks passed
@FraserGreenroyd FraserGreenroyd deleted the Geometry_Engine-#3255-DiffUseGeometryHash branch February 1, 2024 14:31
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 11, 2024
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
3 participants