Skip to content
This repository has been archived by the owner on Mar 23, 2024. It is now read-only.

Implement UnitConverter class #35

Merged
merged 15 commits into from
Oct 7, 2023
Merged

Conversation

chrischang5
Copy link
Contributor

@chrischang5 chrischang5 commented Sep 30, 2023

Description

  • Created UnitConverter class, __init__ and convert functions
  • Created ScalarOrArray type

Verification

  • Added tests in tests/unit/common/test_unit_conversions.py

Resources

@chrischang5
Copy link
Contributor Author

I really spelled UnitConvertor in my commit messages LOL my bad.

@chrischang5 chrischang5 self-assigned this Sep 30, 2023
@chrischang5 chrischang5 marked this pull request as ready for review September 30, 2023 04:43
@chrischang5 chrischang5 added the physics-engine Issue for physics engine label Sep 30, 2023
@chrischang5
Copy link
Contributor Author

chrischang5 commented Sep 30, 2023

One step that could potentially be done to make a more complete test coverage is to parameterize tests for all unit conversions. Currently, only the angle to radians and radians to angle tests are parameterized in this way.

@chrischang5 chrischang5 added enhancement New feature or request and removed physics-engine Issue for physics engine labels Sep 30, 2023
Copy link
Contributor

@DFriend01 DFriend01 left a comment

Choose a reason for hiding this comment

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

Looks great! There was one more thing that I wanted to check, and that was to see if the unit converter works with array-like objects as well as scalars.

Looking at the code, it probably should. Could you do the following:

  • Add some tests to see if passing in ND arrays works where the conversion happens elementwise
  • Change the typing from Scalar to ArrayLike for both the UnitConverter and the ConversionFactor class as appropriate? Take a look at the ArrayLike type. It covers both scalars and arrays in a single type.

@chrischang5 chrischang5 requested a review from DFriend01 October 5, 2023 03:33
Copy link
Contributor

@DFriend01 DFriend01 left a comment

Choose a reason for hiding this comment

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

Good work. Just one small comment but I'm approving now. Once you make the change, feel free to merge this PR.

boat_simulator/common/unit_conversions.py Outdated Show resolved Hide resolved
@chrischang5 chrischang5 merged commit b80d289 into main Oct 7, 2023
@chrischang5 chrischang5 deleted the user/chrischang5/7-unit-conversion branch October 7, 2023 23:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create unit conversion class
2 participants