BHoM / BHoM_Engine

Internal manipulation of the BHoM
GNU Lesser General Public License v3.0
26 stars 13 forks source link

Diffing_Engine: Hash and Diff to leverage new GeometryHash #3268

Closed alelom closed 7 months ago

alelom commented 7 months ago

NOTE: Depends on

https://github.com/BHoM/BHoM/pull/1594

Issues addressed by this PR

Closes https://github.com/BHoM/BHoM_Engine/issues/3255 Closes #3269

The following was also implemented in this PR in order to be able to test properly the implementation of #3255. Closes https://github.com/BHoM/BHoM_Engine/issues/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. - Before (all on `develop`): ![2024-01-29 20_45_06-C__Users_alombardi_Documents_GitHub_DiffingTests_Prototypes_DiffingProfiling_bin](https://github.com/BHoM/DiffingTests_Prototypes/assets/6352844/cb78064b-beda-4769-8555-443ea20e0c8b) - When on this PR [(BHoM_Engine #3268)](https://github.com/BHoM/BHoM_Engine/pull/3268): ![2024-01-29 20_24_42-Select C__Users_alombardi_Documents_GitHub_DiffingTests_Prototypes_DiffingProfil](https://github.com/BHoM/DiffingTests_Prototypes/assets/6352844/31ae8d20-0e08-489d-b7ca-80c63e239eab) 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 https://github.com/BHoM/BHoM_Engine/pull/3257, but calling BH.Engine.Base.Hash() instead of BH.Engine.Geometry.GeometryHash().

Please also run all tests in https://github.com/BHoM/DiffingTests_Prototypes/pull/18. They should all pass. Please note that the default value of the new GeometryHash option is set to true in the ComparisonConfig (see https://github.com/BHoM/BHoM/pull/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

pawelbaran commented 7 months ago

@BHoMBot check compliance

bhombot-ci[bot] commented 7 months ago
@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`
pawelbaran commented 7 months ago

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

alelom commented 7 months ago

@BHoMBot check required

bhombot-ci[bot] commented 7 months ago
@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 commented 7 months ago

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

bhombot-ci[bot] commented 7 months ago
@FraserGreenroyd to confirm, the following actions are now queued: - check `copyright-compliance` - check `dataset-compliance`
FraserGreenroyd commented 7 months ago

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

bhombot-ci[bot] commented 7 months ago
@FraserGreenroyd I have provided neutral checks to the checks requested. These checks will need to be run properly to obtain full results.
bhombot-ci[bot] commented 7 months ago
@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