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

948 add support to create coordinate systems from homogeneous transformation matrices #949

Conversation

mbwinkler
Copy link
Contributor

@mbwinkler mbwinkler commented Nov 3, 2024

Changes

Added support for homogeneous transformation matrices.

Related Issues

Closes #948

Checks

  • updated CHANGELOG.md
  • updated tests/
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

Sorry, something went wrong.

Copy link

github-actions bot commented Nov 3, 2024

Test Results

2 188 tests  ±0   2 187 ✅ ±0   2m 5s ⏱️ +2s
    1 suites ±0       1 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit f600bcd. ± Comparison against base commit 8a34f2d.

♻️ This comment has been updated with latest results.

@mbwinkler
Copy link
Contributor Author

Will update tests/ and doc/ and write an example for the notebook sometime this week.

@mbwinkler mbwinkler self-assigned this Nov 3, 2024
@CagtayFabry CagtayFabry added the transformations everything related to the LCS / CSM label Nov 4, 2024
@CagtayFabry
Copy link
Member

It looks good to me but is still missing tests obviously.
The test suit should be back up running after I merge #896 tomorrow

@CagtayFabry
Copy link
Member

the pytest actions are running again @mbwinkler , you can add some testcases 👍

@CagtayFabry
Copy link
Member

Don't bother with mypy for now @mbwinkler , it can be quite a hassle

@mbwinkler
Copy link
Contributor Author

@CagtayFabry
Wouldn't be a hassle if npt.ArrayLike would actually mean "like an array" and not "can be constructed into an array"...

https://stackoverflow.com/a/76271691

I could change the type to just use np.array and then it would work. Right now mypy just tells me that the 3D indexing (obviously) would not work if I passed e.g. a simple str.

@CagtayFabry
Copy link
Member

@CagtayFabry Wouldn't be a hassle if npt.ArrayLike would actually mean "like an array" and not "can be constructed into an array"...

https://stackoverflow.com/a/76271691

I could change the type to just use np.array and then it would work. Right now mypy just tells me that the 3D indexing (obviously) would not work if I passed e.g. a simple str.

I think np.array should be sufficient here in that case

very good find on the SO post 👍

Comment on lines 760 to 762
translation = np.resize(
self.coordinates.data.to(translation_unit).m, (time_dim, 3)
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently this expression raises a mypy error, because using self.coordinates could return a TimeSeries, which in turn would return pint.Quantity | MathematicalExpression when .data is accessed and the latter one does not support unit transformations. Not sure how to fix this.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave out supporting TimeSeries for now (throwing an error).

We could add

if isinstance(self.coordinates, TimeSeries):
    raise NotImplementedError("Cannot convert LCS with `TimeSeries` coordinates to homogeneous matrix")

at the beginning

Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

thanks for adding the test cases!

I think this can be set as "ready for review"? :)

Comment on lines 760 to 762
translation = np.resize(
self.coordinates.data.to(translation_unit).m, (time_dim, 3)
)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave out supporting TimeSeries for now (throwing an error).

We could add

if isinstance(self.coordinates, TimeSeries):
    raise NotImplementedError("Cannot convert LCS with `TimeSeries` coordinates to homogeneous matrix")

at the beginning

@mbwinkler mbwinkler marked this pull request as ready for review November 6, 2024 17:13
@CagtayFabry
Copy link
Member

pytest windows failures unrelated

@CagtayFabry CagtayFabry merged commit e0e3a65 into master Nov 6, 2024
27 of 29 checks passed
@CagtayFabry CagtayFabry deleted the 948-add-support-to-create-coordinate-systems-from-homogeneous-transformation-matrices branch November 6, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transformations everything related to the LCS / CSM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to create Coordinate Systems from homogeneous transformation matrices
2 participants