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

Update UnitsNet to fix CoefficientOfThermalExpansion conversions #81

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

JosefTaylor
Copy link
Contributor

NOTE: Depends on

Issues addressed by this PR

Closes #80

Test files

Changelog

Additional comments

@JosefTaylor JosefTaylor added the type:bug Error or unexpected behaviour label Sep 7, 2022
@JosefTaylor JosefTaylor self-assigned this Sep 7, 2022
@JosefTaylor
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 7, 2022

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

There are 19 requests in the queue ahead of you.

@IsakNaslundBh
Copy link
Contributor

@BHoMBot check unit-test

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 8, 2022

@IsakNaslundBh sorry, I didn't understand.
Was that comment an instruction for me? If so, could you state again what check you would like me to do?
For a list of available instructions, please see this wiki page.

@IsakNaslundBh
Copy link
Contributor

@BHoMBot check unit-tests

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 8, 2022

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

  • check unit-tests

There are 1 requests in the queue ahead of you.

@JosefTaylor
Copy link
Contributor Author

image
Based on testing of the methods failing in unit-tests, I believe the test should be dispensated. The differences that bhom-bot shows other than inverse degrees are rounding errors.

@FraserGreenroyd
Copy link
Contributor

Item 9, 35.314667 compared to 35.314725 isn't a rounding error - that's 0.000058 different. Whoever setup the unit test decided we need to go to 6dp - so question is, did the new UnitsNet introduce differences at that level of precision that didn't exist previously?

@IsakNaslundBh
Copy link
Contributor

IsakNaslundBh commented Sep 9, 2022

Did some quick digging on the UnitsNet repo, and 9 and 14 are for example handled by angularsen/UnitsNet#1029 .

And the moment issues seem to stem from angularsen/UnitsNet#1011

These are probably both improvements (from looking at the issues on UnitsNet I do agree with the sentiment of the changes there) , but it needs to be validated, and if accepted, UnitTests ofc updated.

@FraserGreenroyd I assume this is ok to get merged in next sprint, given that it fixes the bug shown in item (1) in Josefs list?

@FraserGreenroyd
Copy link
Contributor

@FraserGreenroyd I assume this is ok to get merged in next sprint, given that it fixes the bug shown in item (1) in Josefs list?

Correct, next sprint can still merge bug fixes.

@FraserGreenroyd
Copy link
Contributor

@BHoMBot check versioning

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 13, 2022

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

  • check versioning

@JosefTaylor
Copy link
Contributor Author

Doing some comparison with a user-calculated conversion based on official conversions and earth gravity:
image
The PR gives us better fidelity to the user calculation than the main branch

@JosefTaylor
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 13, 2022

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

There are 58 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 13, 2022

The check project-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@JosefTaylor
Copy link
Contributor Author

@FraserGreenroyd does this look good to you?

@IsakNaslundBh
Copy link
Contributor

Doing some comparison with a user-calculated conversion based on official conversions and earth gravity: image The PR gives us better fidelity to the user calculation than the main branch

Perfect @JosefTaylor .

With that, we just need to get the UnitTests updated, so they also reflect this change. Have sent you a script to help with this!

@JosefTaylor
Copy link
Contributor Author

@BHoMBot check unit-tests

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

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

  • check unit-tests

There are 9 requests in the queue ahead of you.

@JosefTaylor
Copy link
Contributor Author

@BHoMBot check compliance

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

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

There are 12 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

Please be advised that the check with reference 8351472869 has more than 50 annotations of notes. API limitations restrict annotations to 50. You may need to rerun this check to obtain the next set when you make changes. At the time of reporting this check, there are 198 additional annotations waiting, made up of 0 errors and 198 warnings.

@JosefTaylor
Copy link
Contributor Author

@BHoMBot check required

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

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

There are 13 requests in the queue ahead of you.

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

The check project-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

Copy link
Contributor

@FraserGreenroyd FraserGreenroyd left a comment

Choose a reason for hiding this comment

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

Have reviewed with the unit tests and discussion here, as well as an on Teams discussion with @JosefTaylor and am happy with the updates this PR is bringing from a code review and deployment perspective for the upcoming beta. Thanks for updating the unit tests following the review @JosefTaylor 😄

@JosefTaylor
Copy link
Contributor Author

@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

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

  • check ready-to-merge

@FraserGreenroyd
Copy link
Contributor

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

@bhombot-ci
Copy link

bhombot-ci bot commented Sep 14, 2022

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

@FraserGreenroyd FraserGreenroyd merged commit 5de42d5 into main Sep 14, 2022
@FraserGreenroyd FraserGreenroyd deleted the Units_Engine-#80-update-UnitsNet branch September 14, 2022 14:58
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
Development

Successfully merging this pull request may close these issues.

Coefficient of Thermal Expansion is converted incorrectly
3 participants