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

BHoM_Engine: rounding issues fixed #3430

Merged
merged 7 commits into from
Nov 5, 2024

Conversation

pawelbaran
Copy link
Member

@pawelbaran pawelbaran commented Nov 4, 2024

Issues addressed by this PR

Closes #3426
Closes #3429

Test files

On SharePoint, UTs added

Changelog

Additional comments

So far we had just one method, RoundWithTolerance, which did round the numbers towards the floor regardless of sign, i.e. 121 rounded to 2 would yield 120 and equally, -121 rounded to 2 would yield -120. This is potentially ambiguous on 2 levels:

  • the name does not indicate the value is rounded to floor
  • rounding to floor is not consistent with what general rounding does, i.e. rounding to floor should always return lower value from the 2 candidates (floor and ceiling) regardless of sign (so -121 should return -122)

As a remedy I propose 3 separate methods that follow the standard rounding conventions:

  • RoundToFloor
  • Round
  • RoundToCeiling

I believe this is a right move for the general audience, although I understand you may need the current implementation for diffing @alelom - if so, I'd suggest moving it to Diffing_Engine as a private method. Let's catch up on that 👍

@pawelbaran pawelbaran added the type:bug Error or unexpected behaviour label Nov 4, 2024
@pawelbaran pawelbaran requested a review from alelom November 4, 2024 16:00
@pawelbaran pawelbaran self-assigned this Nov 4, 2024
pawelbaran added a commit that referenced this pull request Nov 4, 2024
@pawelbaran
Copy link
Member Author

@BHoMBot check compliance
@BHoMBot check installer
@BHoMBot check versioning

Copy link

bhombot-ci bot commented Nov 4, 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
  • check installer
  • check versioning

alelom added a commit to BHoM/DiffingTests_Prototypes that referenced this pull request Nov 5, 2024
Co-Authored-By: Pawel Baran <pawel.baran@burohappold.com>
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

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

Tested with BHoM/DiffingTests_Prototypes#22 and agreed the change with @pawelbaran.

@pawelbaran
Copy link
Member Author

@BHoMBot check required

Copy link

bhombot-ci bot commented Nov 5, 2024

@pawelbaran 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

Copy link

bhombot-ci bot commented Nov 5, 2024

FAO: @adecler @pawelbaran @IsakNaslundBh
@pawelbaran is seeking dispensation on this Pull Request to skip a required check. Please can you provide authorisation for the check to be skipped, or provide assistance as appropriate.

The check they wish to have dispensation on is documentation-compliance.

If you are providing dispensation on this occasion, please reply with:

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 32526179440

@pawelbaran
Copy link
Member Author

@BHoMBot check unit-tests
@BHoMBot check dataset-compliance

Copy link

bhombot-ci bot commented Nov 5, 2024

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

  • check unit-tests
  • check dataset-compliance

@pawelbaran
Copy link
Member Author

Dispensating documentation compliance as it is out of scope of this PR

@pawelbaran
Copy link
Member Author

@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 32526179440

Copy link

bhombot-ci bot commented Nov 5, 2024

@pawelbaran I have now provided a passing check on reference 32526179440 as requested.

@pawelbaran
Copy link
Member Author

@BHoMBot check copyright-compliance

Copy link

bhombot-ci bot commented Nov 5, 2024

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

  • check copyright-compliance

There are 7 requests in the queue ahead of you.

@pawelbaran
Copy link
Member Author

@BHoMBot check ready-to-merge

Copy link

bhombot-ci bot commented Nov 5, 2024

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

  • check ready-to-merge

@pawelbaran pawelbaran merged commit 0cf0a98 into develop Nov 5, 2024
12 checks passed
@pawelbaran pawelbaran deleted the BHoM_Engine-#3426-RoundingIssues branch November 5, 2024 11:51
pawelbaran added a commit to BHoM/documentation that referenced this pull request Nov 5, 2024
alelom pushed a commit to BHoM/documentation that referenced this pull request Nov 5, 2024
pawelbaran added a commit that referenced this pull request Nov 5, 2024
@BHoMBot BHoMBot mentioned this pull request Dec 9, 2024
alelom added a commit to BHoM/DiffingTests_Prototypes that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error or unexpected behaviour
Projects
None yet
2 participants