search

SymbolicML / DynamicQuantities.jl

Efficient and type-stable physical quantities in Julia
https://symbolicml.org/DynamicQuantities.jl/dev/
Apache License 2.0
120 stars 15 forks source link

`bionomial` and `factorial` functions not defined? #124

Closed TorkelE closed 2 months ago

TorkelE commented 3 months ago

I'm coming here indirectly via ModelingToolkit.jl (which my package, Catalyst.jl, depends on) which recently started using DynamicQuantities.

Catalyst currently crashed whenever you try to use the binomial or factorial functions. I have traced this to ModelingToolkit: https://github.com/SciML/ModelingToolkit.jl/issues/2554. I am not familiar with DynamicQuantities (only ModelingToolkit), but it seems like this issue might be traced here? (see conversation)

Sorry for the non-exhaustive description. This sounds like the error (and how to fix it) might be directly obvious for someone familiar with the package. If not, I will try to figure out more about what's going on.

MilesCranmer commented 3 months ago

Yes, these functions are not defined. We would need to add them to src/units.jl. However, are you sure it makes sense to pass quantities to these functions? To me it seems that the behavior is ambiguous. Should factorial(10u"m") be different dimensionality than factorial(10u"km")? I’m not sure.

Maybe describe your expected behavior here, as these are going to be physical continuous quantities rather than integers.

MilesCranmer commented 3 months ago

Or, maybe you just want to define them for dimensionless input? In which case, add it to this list: https://github.com/SymbolicML/DynamicQuantities.jl/blob/25e55a1148d460ac6f810d0e8db88cd64f159f20/src/math.jl#L156-L167

TorkelE commented 3 months ago

I think, right now, the input would be dimensionless. I will make a PR to add them there.

(Technically, dimensional input could be added. Not really sure what I would expect though. We kind of jack into MTK which actually does this for us. Personally, I am not really using the unit feature, but it was something that suddenly started throwing errors through MTK)

MilesCranmer commented 3 months ago

Dimensionless sounds good to me.

MilesCranmer commented 2 months ago

Fixed with #125