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

fix: mode signature return types #3153

Merged
merged 11 commits into from
Feb 21, 2024
Merged

Conversation

rich-martinez
Copy link
Contributor

@rich-martinez rich-martinez commented Feb 10, 2024

Reason(s) For Change

  • Using mode with and array of numbers seems to always return array of numbers regardless of how many modes are found
  • The mode function seems to only ever return an array:
    /**
    * Calculates the mode in an 1-dimensional array
    * @param {Array} values
    * @return {Array} mode
    * @private
    */
    function _mode (values) {
    values = flatten(values.valueOf())
    const num = values.length
    if (num === 0) {
    throw new Error('Cannot calculate mode of an empty array')
    }
    const count = {}
    let mode = []
    let max = 0
    for (let i = 0; i < values.length; i++) {
    const value = values[i]
    if (isNumeric(value) && isNaN(value)) {
    throw new Error('Cannot calculate mode of an array containing NaN values')
    }
    if (!(value in count)) {
    count[value] = 0
    }
    count[value]++
    if (count[value] === max) {
    mode.push(value)
    } else if (count[value] > max) {
    max = count[value]
    mode = [value]
    }
    }
    return mode
    }

Changes

  • Update the mode signatures return types to be an array of current types
  • Add mode type tests

Notes

It is unclear if this type needs to change as well, because it seems like it should return an array on done:

mode(this: MathJsChain<MathType[]>): MathJsChain<MathType[]>

@josdejong
Copy link
Owner

Thanks! Your fixes are indeed correct.

It is unclear if this type needs to change as well, because it seems like it should return an array on done:

mode(this: MathJsChain<MathType[]>): MathJsChain<MathType[]>

I think this type is wrong: in chained function, you can only pass a single Matrix. At first glance I would expect the type to be this:

mode(this: MathJsChain<MathCollection>): MathJsChain<MathCollection>

And the same for the surrounding functions max, mean, median, min, prod. Except for prod, they just have a redundant type definition, the MathCollection variant is there for all but prod.

@rich-martinez
Copy link
Contributor Author

@josdejong
Copy link
Owner

Thanks Rich, looks good 👌

@josdejong josdejong merged commit b78ac81 into josdejong:develop Feb 21, 2024
9 checks passed
@josdejong
Copy link
Owner

Published in v12.4.0, thanks again.

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

Successfully merging this pull request may close these issues.

None yet

2 participants