fix: use utility isNaN for consistent max and min results

[orelbn]

Addresses: #3387

Description:

• Uses the isNaN utility function to determine if a value isNaN and then sets the value to the results if it isNaN.
  • Assumes that the larger and smaller functions will return false when comparing any value that isNaN, so that the result will not be changed
• Adds corresponding tests for BigNumber and Unit
• Changes the error message in tests due to failure at an earlier stage.

Additional Notes:

I don't think this is necessarily a big issue because you would have to be comparing a Unit/BigNumber that has a value that is NaN. I think having undefined behaviour for that might be okay or throwing an error (so let me know if you decide not to address the issue).

AI Summary:

This pull request includes changes to the max and min functions to improve their handling of NaN values and adds corresponding unit tests. The key changes involve updating dependencies, modifying the logic for handling NaN values, and enhancing test coverage.

Enhancements to max and min functions:

• src/expression/transform/max.transform.js: Added isNaN to the dependencies of createMaxTransform.
• src/expression/transform/min.transform.js: Added isNaN to the dependencies of createMinTransform.
• src/function/statistics/max.js: Added isNaN to the dependencies of createMax and modified the logic to handle NaN values correctly. [1] [2]
• src/function/statistics/min.js: Added isNaN to the dependencies of createMin and modified the logic to handle NaN values correctly. [1] [2]

Unit tests:

• test/unit-tests/function/statistics/max.test.js: Added tests to verify the handling of NaN values in the max function. [1] [2] [3]
• test/unit-tests/function/statistics/min.test.js: Added tests to verify the handling of NaN values in the min function. [1] [2] [3]

[gwhitney]

Thanks very much for the PR! Generally, it looks solid. Just two items from review:

1. Since this PR solidifies and makes explicit, as a dependency for the proper behavior of max and min, a particular feature of the behavior of smaller and larger (that they return 'false' when either argument is not-a-number), that feature of smaller and larger needs to be (a) documented, and (b) unit-tested (there is currently no mention of NaN in the unit tests for either smaller or larger). These things should happen in this PR; please update.

2. There is an internal code design issue that @josdejong will need to weigh in on before this is merged. The difficulty is that mathjs has a method isNaN with the same name as a JavaScript built-in. That overload is not really a problem from the client's point of view: they can use math.isNaN or built-in isNaN as they please. The problem arises in factories that depend on isNaN. In their implementing code, the injected isNaN shadows the built-in, making the implementation potentially confusing to read, and making it tricky to call the built-in instead if desired. (Note there is a Number.isNaN but it is not quite identical to built-in isNaN because the latter does conversions to number but the former does not.)

Such injections of isNaN are not new (prior to this PR, they occur in mode, partitionSelect, and variance, all in the "statistics" section, whereas built-in isNaN is used in fraction, hasNumericValue, norm, range, simplifyConstant, sqrt, and typed). However, as this PR would almost double the occurrences of these injections, if there were an inclination to address the potential concern here, this PR might be a good opportunity to do so.

Personally, I see three possible ways the conflict could be eliminated: (A) have the internal factory be called something like is_nan for factory dependencies, but export it as isNaN in the bundle for backwards compatibility; (B) rename isNaN to something else, like isnan (where casing is used to distinguish the builtin and the mathjs function, as with typeof and typeOf), presumably leaving isNaN as a backwards-compatibility deprecated formerly setting until the next breaking-change point; or (C) leave the factory and bundle alone, but in these implementations where the arguments to the factory are being destructured, write something like mathIsNaN: isNaN in place of just isNaN so that in the implementations of the max, min, mode, partitionSelect, and variance factories, they can refer to the mathjs method isNaN locally via mathIsNaN rather than the potentially confusing isNaN.

If @josdejong does want to steer away from using injected isNaN in factory implementation code, then (C) is the path of least resistance, but does not prevent such injections from creeping back in, whereas (A) or (B) does. But maybe this is a small and uncommon enough problem that just creating a pattern of doing (C) addresses it enough -- or maybe the decision will be it does not need attention at all.

[orelbn]

Thanks very much for the PR! Generally, it looks solid. Just two items from review:

1. Since this PR solidifies and makes explicit, as a dependency for the proper behavior of max and min, a particular feature of the behavior of smaller and larger (that they return 'false' when either argument is not-a-number), that feature of smaller and larger needs to be (a) documented, and (b) unit-tested (there is currently no mention of NaN in the unit tests for either smaller or larger). These things should happen in this PR; please update.
2. There is an internal code design issue that @josdejong will need to weigh in on before this is merged. The difficulty is that mathjs has a method isNaN with the same name as a JavaScript built-in. That overload is not really a problem from the client's point of view: they can use math.isNaN or built-in isNaN as they please. The problem arises in factories that depend on isNaN. In their implementing code, the injected isNaN shadows the built-in, making the implementation potentially confusing to read, and making it tricky to call the built-in instead if desired. (Note there is a Number.isNaN but it is not quite identical to built-in isNaN because the latter does conversions to number but the former does not.)

Such injections of isNaN are not new (prior to this PR, they occur in mode, partitionSelect, and variance, all in the "statistics" section, whereas built-in isNaN is used in fraction, hasNumericValue, norm, range, simplifyConstant, sqrt, and typed). However, as this PR would almost double the occurrences of these injections, if there were an inclination to address the potential concern here, this PR might be a good opportunity to do so.

Personally, I see three possible ways the conflict could be eliminated: (A) have the internal factory be called something like is_nan for factory dependencies, but export it as isNaN in the bundle for backwards compatibility; (B) rename isNaN to something else, like isnan (where casing is used to distinguish the builtin and the mathjs function, as with typeof and typeOf), presumably leaving isNaN as a backwards-compatibility deprecated formerly setting until the next breaking-change point; or (C) leave the factory and bundle alone, but in these implementations where the arguments to the factory are being destructured, write something like mathIsNaN: isNaN in place of just isNaN so that in the implementations of the max, min, mode, partitionSelect, and variance factories, they can refer to the mathjs method isNaN locally via mathIsNaN rather than the potentially confusing isNaN.

If @josdejong does want to steer away from using injected isNaN in factory implementation code, then (C) is the path of least resistance, but does not prevent such injections from creeping back in, whereas (A) or (B) does. But maybe this is a small and uncommon enough problem that just creating a pattern of doing (C) addresses it enough -- or maybe the decision will be it does not need attention at all.

Thanks for the review! I will add more tests and update the documentation appropriately. Regarding your second point, I agree it can be confusing, and I will leave it to Jos to decide how he wants to proceed. For now, I will use option C as a placeholder in this particular implementation, as I don't see it causing any side effects.

[gwhitney]

Excellent, thank you! Just awaiting @josdejong's decision on how mathjs should handle isNaN shadowing internally before final review.