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 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.

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

@gwhitney I think this PR is in line with what we discussed in #3374 and ready to merge, agree?

@gwhitney I think this PR is in line with what we discussed in #3374 and ready to merge, agree?

So @orelbn employed my suggestion of avoiding the shadowing of built-in isNaN by mapping the injected isNaN to mathIsNaN in the factory implementation. If you think this is the solution of choice for the potential shadowing problem, then perhaps this PR should make the analogous renaming in the other three statistics functions that inject isNaN. I made two other suggestions above about dealing with this shadowing concern; if you like either of them better then perhaps this PR should embark on one of them? Otherwise yes this PR seems ready to go.

I think the solution of using mathIsNaN to prevent shadowing isNaN is the best option in the current situation. Thanks for bringing this up Glen. In the long term we may want to rename the function to prevent this shadowing issue, but let's postpone that and keep this PR focused on fixing the min/max bug.

I'll merge the PR as it is now, and solve the other occurrences of shadowing isNaN the same way as in this PR.

Published now in v14.3.0