feat: adds unit support to ceil, floor, and fix functions

[orelbn]

Description

• adds unit support to ceil, floor, and fix functions -> Addresses Support Units in Ceil and Floor #3216

Test

• Added automated unit-tests

Additional Notes

• I wasn't able to run the type tests locally. Can you please verify that they are passing properly?

[josdejong]

Thanks Orel! This is exactly what we're looking for, nice job. Thanks for updating all the docs and TypeScript types. The types run fine, they run via Github Actions of this PR and all checks are green.

I've made one inline comment, can you have a look at that?

[orelbn]

Is there a restriction that the matrix or array must contain only units? I believe that to be the case. If so, I would like to update the types further. I would like to do it in a separate PR.

[josdejong]

Thanks for the updates. I made two more comments, can you have a look at those?

Is there a restriction that the matrix or array must contain only units?

Matrices can contain mixed content of numbers, units, complex numbers etc. There is still an issue in that regard though: the type definitions currently do not allow to have a matrix containing units at all:

mathjs/types/index.d.ts

Line 16 in fb76ac4

export type MathArray = MathNumericType[] | MathNumericType[][] // TODO: MathArray can also contain Unit

Maybe best to address that issue separately though.

[orelbn]

Thanks for the updates. I made two more comments, can you have a look at those?

Is there a restriction that the matrix or array must contain only units?

Matrices can contain mixed content of numbers, units, complex numbers etc. There is still an issue in that regard though: the type definitions currently do not allow to have a matrix containing units at all:

mathjs/types/index.d.ts

Line 16 in fb76ac4

export type MathArray = MathNumericType[] | MathNumericType[][] // TODO: MathArray can also contain Unit

Maybe best to address that issue separately though.

Thank you for the clarification! Is the desired behaviour that matrices and arrays only contain units when the second or third argument to the ceil, floor, fix, or round function is a unit?

For example, round([1], unit('cm')) throws an error, as does round([1, unit('2 cm')], 2, unit('cm')), but that is not reflected in the current type definition.

mathjs/types/index.d.ts

Line 1192 in 6bcb26c

round<U extends MathCollection>(x: U, n: number | BigNumber, unit: Unit): U

Proposed new type definition:

round(Unit[] | Unit[][] | UnitMatrix, number | BigNumber, Unit)
round(Unit[] | Unit[][] | UnitMatrix, Unit)

[josdejong]

Is the desired behaviour that matrices and arrays only contain units when the second or third argument to the ceil, floor, fix, or round function is a unit?

That makes sense indeed! Can you refine the type definitions with your proposal?

[orelbn]

Is the desired behaviour that matrices and arrays only contain units when the second or third argument to the ceil, floor, fix, or round function is a unit?

That makes sense indeed! Can you refine the type definitions with your proposal?

I played around with it a bit.

This is what I believe the tasks that are required:

1. Expand the Matrix and MathArray types to accept Units.
2. Add a utility for checking whether an Array is a UnitArray and Matrix is a UnitMatrix

From a user perspective, if they create an array that contains multiple types they will get this error

  const array = []
  array.push(u1)
  array.push(u3)
  array.push(1)
  array.pop()

  math.ceil(array, 1, math.unit('cm'))

They can use the utility (although slow since it has to go through. the entire array/matrix) or assert the type.

if (isUnitArray(array)) math.ceil(array, 1, math.unit('cm'))
or
math.ceil(array as UnitArray, 1, math.unit('cm'))

[josdejong]

Thanks for the updates!

The improved TypeScript support is awesome, and with the generic MathCollection<T> we can refine a lot more of the type definitions. However we're already getting out of scope of this PR so let's limit what we do in this PR 😅.

Two questions:

1. There are some subtle changes in the existing type definitions (like for multiply). Are you sure there are no breaking changes in the types?

2. Thinking aloud here: I'm a bit in doubt about the typeguards isUnitArray and isUnitMatrix as they are. I think we can use more fundamental helper functions to determine whether a matrix/array contains a single data type (i.e. is homogeneous). Not just for units but for any type. So we would need something more generic like isHomogeneousArray(array, isComplex) or isArrayOfType(isComplex). The ideal TypeScript notation would be something like isArray<Complex>(). Maybe we should implement it a method for this on DenseMatrix and SparseMatrix which verifies the contents and sets the internal datatype (which optimizes evaluation) and sets the TypeScript type.

   In short: this needs some more thinking-through, and maybe it is best to keep that out of scope of this PR and address in a separate discussion. Until then, it would be best not to export new helper functions like isUnitArray as long as we're not sure what the final API will be. What do you think?

[orelbn]

Thanks for the updates!

The improved TypeScript support is awesome, and with the generic MathCollection<T> we can refine a lot more of the type definitions. However we're already getting out of scope of this PR so let's limit what we do in this PR 😅.

Two questions:

1. There are some subtle changes in the existing type definitions (like for multiply). Are you sure there are no breaking changes in the types?
2. Thinking aloud here: I'm a bit in doubt about the typeguards isUnitArray and isUnitMatrix as they are. I think we can use more fundamental helper functions to determine whether a matrix/array contains a single data type (i.e. is homogeneous). Not just for units but for any type. So we would need something more generic like isHomogeneousArray(array, isComplex) or isArrayOfType(isComplex). The ideal TypeScript notation would be something like isArray<Complex>(). Maybe we should implement it a method for this on DenseMatrix and SparseMatrix which verifies the contents and sets the internal datatype (which optimizes evaluation) and sets the TypeScript type.
   In short: this needs some more thinking-through, and maybe it is best to keep that out of scope of this PR and address in a separate discussion. Until then, it would be best not to export new helper functions like isUnitArray as long as we're not sure what the final API will be. What do you think?

Sorry for the delayed response. Very busy this past week.

All of your suggestions sound great. Thank you for the input 😄 ! I will remove the type guards and address your suggestion in a separate PR. I agree that there is definitely more thinking to do.

Regarding the multiply, I will take a look again.

After further look:
There are breaking changes, so we will want to see how we want to handle that.

Not all array operations are supported on unit arrays, so if we say that MathArrays can include units.
The above code won't capture the first two definitions of the multiply function since it expects MathArray to translate down to MathNumericType[] => but in the current PR, I said that MathArray translates down to MathScalarType[]

  multiply<T extends MathNumericType[]>(x: T, y: T[]): T
  multiply<T extends MathNumericType[]>(x: T[], y: T): T

So what I can do for now is set the default to a MathNumerticType<T extends MathScalarType = MathNumericType>, but you can explicitly still set it to MathScalarType .

I will address the type guard sometime in the next couple of days.

[orelbn]

I did find a small error in one of the multiply function types. Would you like me to file an issue or fix it in one of the upcoming PRs?

Error in function type: multiplication of two MathArrays does not necessarily result in a MathArray.

mathjs/types/index.d.ts

Line 1364 in 07bff55

multiply<T extends MathArray>(x: T, y: T): T

To Reproduce
Steps to reproduce the behavior.

Multiply two single-dimensional arrays together, and the result is a number.

[josdejong]

Thanks for the updates, I think we're there! I made one last small remark, can you have a look at that?

Good find about the wrong types when multiplying two vectors. In that case mathjs returns a scalar value, and not an array containing one value. Since this is not a regression introduced in this PR but an existing bug in the type definitions, I think it's best to create a separate PR improving the types for vector multiplication, ok?

I've had a thorough look at the changes in the type definitions. I expect the changes are not be breaking in normal, practical cases, but there may be tricky edge cases that have breaking behavior. To be careful in this regard, I would like to merge this PR into the next major version v14, ok? (I plan to release v14 "soonish", within a couple of weeks).

[orelbn]

ad a thorough look at the changes in the type definitions. I expect the changes are not be breaking in normal, practical cases, but there may be tricky edge cases that have breaking behavior. To be careful in this reg

That sounds good. I will address the vector multiplication types separately.

Adding the functionality in version 14 is completely fine with me.

Thank you for the in-depth review 😄

[josdejong]

Thanks! Merged now in the v14 branch.

[josdejong]

Published now in v14.0.0. Thanks again Orel.