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

List.MaximumItem breaks on mixed input #9009

Closed
sunfield opened this issue Jul 23, 2018 · 2 comments
Closed

List.MaximumItem breaks on mixed input #9009

sunfield opened this issue Jul 23, 2018 · 2 comments

Comments

@sunfield
Copy link

Dynamo version

2.0.1.5055

Operating system

Windows 10

What did you do?

Took a list of numbers from a file (so as strings), sorted and converted to number, then tried to find the largest one using List.MaximumItem.

What did you expect to see?

The largest value in the list returned.

What did you see instead?

null, with a warning.

Boiled it down to this:
tonumber

Tests reveal that if some numbers have a decimal separator and others don't, then the output from String.ToNumber is not uniform, which causes problems when using List.MaximumItem, which apparently cannot handle a list that includes e.g. Int and Double.
There is actually test code for this which I imagine should have picked this up:

Assert.AreEqual(66, List.MaximumItem(new List<object> { 8.223, 4, 0.64, 66, 10.2 }));

What's interesting, looking at the source, is that the Max() method, which List.MaximumItem uses, seems to cast its inputs to doubles. My C# is rusty but I'm feel like this should be working?

return list.Max<object, object>(DoubleConverter);

public static double Max(double value1, double value2)

Looking into the DoubleConverter reveals that this was added to fix exactly this problem here #2041, which maybe it did, but perhaps got unfixed with an update..? I'm in over my head at this point, though.

@jnealb
Copy link
Collaborator

jnealb commented Jul 23, 2018

@sunfield Thank you for this report. We have filed it as QNTM-4876 and it is in the 2.1 backlog.

@aparajit-pratap
Copy link
Contributor

@sunfield thanks for highlighting the issue and fix :)
#9012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants