8000 Fix multiplication of arrays with units by Delaney · Pull Request #3456 · josdejong/mathjs · GitHub
[go: up one dir, main page]
Skip to content

Fix multiplication of arrays with units #3456

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Fix multiplication of arrays with units #3456

wants to merge 8 commits into from

Conversation

Delaney
Copy link
@Delaney Delaney commented Apr 19, 2025

Add Unit as an accepted type for complex conjugate conj

@Delaney Delaney mentioned this pull request Apr 19, 2025
Open
@gwhitney
Copy link
Collaborator

Thanks for your effort in opening a pull request for this issue!

  • the new conj implementation for Unit is not correct, as a Unit is allowed to have a Complex value ((300+250i) ohm comes up in circuit analysis, for example). The implementation of conj for a Unit should push the conj operation down to operate on the Unit's value.

  • There needs to be a test for conj on a unit with complex value, and quite possibly also Units with Fraction and BigNumber values as well.

  • Since the instance of multiply in question is really just calling dot, there should be tests of appropriate behavior of dot on vectors and column vectors of Units (including units with Complex values).

Much appreciated.

@gwhitney

This comment was marked as duplicate.

Sign in to view
@josdejong
Copy link
Owner

Thanks for your PR Delaney. Good feedback points Glen 👍.

It's indeed possible to optimize the conj function similar to the dot function, that could be nice but let's please do that in a separate PR. In that PR we also have to make sure we actually improve performance by writing a small benchmark to see the effect of the improvements.

@gwhitney gwhitney mentioned this pull request Apr 23, 2025
Open
@gwhitney
Copy link
Collaborator

OK, having created #3464 I hid the comment about that possible optimization here. @Delaney feel free to proceed with the other feedback (there could conceivably be more in final review, i stopped review when I found the behavior problem).

@Delaney
Copy link
Author
Delaney commented Apr 28, 2025 •
edited
Loading

Thanks for your effort in opening a pull request for this issue!

  • the new conj implementation for Unit is not correct, as a Unit is allowed to have a Complex value ((300+250i) ohm comes up in circuit analysis, for example). The implementation of conj for a Unit should push the conj operation down to operate on the Unit's value.
  • There needs to be a test for conj on a unit with complex value, and quite possibly also Units with Fraction and BigNumber values as well.
  • Since the instance of multiply in question is really just calling dot, there should be tests of appropriate behavior of dot on vectors and column vectors of Units (including units with Complex values).

Much appreciated.

@gwhitney @josdejong The adjustments and test cases have been added. PS: I reckon there's a better way to handle returning the conjugated Unit value than what I'm currently doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Delaney @gwhitney @josdejong
0