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

min does not work correctly in the "eval" form #598

Closed
maciejjaskowski opened this Issue Mar 7, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@maciejjaskowski

maciejjaskowski commented Mar 7, 2016

p = {'p': [4,5]}
math.eval("min([1,2], p)", p)
>> 4

instead of [1,2]

This

math.eval("min([1,2], [4,5])")

works correctly.

This does NOT work either:

p = {'p': [4,5]}
math.min([1,2], p['p'])

but that works:

p = {'p': math.matrix([4,5])}
math.min([1,2], p['p'])
@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 7, 2016

Thanks, that's indeed odd. Something weird going on when passing one Matrix and one Array instead of two Matrices. Will have a look into it asap.

@maciejjaskowski

This comment has been minimized.

maciejjaskowski commented Mar 7, 2016

There is more weirdoes with 'min', e.g.:

math.min(math.matrix([1,2]), math.matrix([4,5])).toArray()
>> [4,5]
@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 10, 2016

I see now that the syntax math.min(matrix, matrix) (element wise minimum) isn't supported at all. So "correct" behavior would be throwing an error. It would be a useful feature though so let's extend min and max and maybe some others as wel.

@maciejjaskowski

This comment has been minimized.

maciejjaskowski commented Mar 10, 2016

I am not sure. Since math.min([[1,2], [3,4]], 1) works then one can stack matrix [1,2] on top of [3,4] to arrive at the result of math.min([1,2], [3,4]). An error message would be very helpful, though.

@bergstrat

This comment has been minimized.

bergstrat commented Mar 15, 2016

Hey guys,

Is this being looked into?
I am looking to contribute to an OS project for school and this seems like a good place to start.

@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 15, 2016

Thanks for your offer @bergstrat, would be greate if you could pick this up. A simple, the short term solution is to add a check on whether the arguments are scalars and if not throw an Error. A nicer solution is to extend the functions like min and max with support for this.

josdejong added a commit that referenced this issue Mar 24, 2016

Throw an error when functions `min`, `max`, `mean`, or `median` are i…
…nvoked with multiple matrices as arguments (see #598)
@josdejong

This comment has been minimized.

Owner

josdejong commented Mar 24, 2016

I've implemented error messages when passing multiple matrices.

@bergstrat still interested in picking extension of these functions this up?

@josdejong josdejong closed this Oct 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment