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

Un-breaking functions requiring <:Real #66

Closed
cscherrer opened this issue Jun 16, 2022 · 16 comments · Fixed by #87
Closed

Un-breaking functions requiring <:Real #66

cscherrer opened this issue Jun 16, 2022 · 16 comments · Fixed by #87

Comments

@cscherrer
Copy link
Contributor

Now that StaticFloat64 <: Real is false, lots of functions that are defined independently of Static.jl are breaking. More concretely, suppose

foo(x::Real)

is defined in a package that does not depend on Static.jl. Passing a StaticFloat64 to foo now throws an MethodError.

For packages I control, there's an easy fix: Change the constraint to ::Number or remove it altogether. But what's the recommended approach for packages I can't change so easily?

The only easy fix I see is to define a local _foo(x) = foo(x), add a method _foo(::StaticFloat64{x}) where {x}, and then always call _foo. But that seems like a mess, and it will still break downstream when users of my package try to call foo. Any other ideas?

@Tokazama
Copy link
Collaborator

We didn't subtype Real because of some invalidations I kept encountering with StaticInt. There are probably some that would still happen for StaticFloat64 but I doubt it would be as many.

The problem with having an independent type hierarchy is that we get a large decrease in load time b/c we can pool a bunch of definitions under StaticNumber. That and the many trig generated methods that StaticFloat64 uses have made me think it might be worth having StaticFloats and StaticIntegers as two packages that are loaded by Static. Then we could just use StaticIntegers for ArrayInterface and not worry about upsetting people with all the added things that float support gives use.

Or maybe I haven't slept in 36 hours and that's just a bad idea.

@cscherrer
Copy link
Contributor Author

Thanks @Tokazama . I still think support for this should just go into Base. We already have type-level support for irrationals that seems to work very well. As I understand, the argument for having type-level support for irrationals is that it seems to work well, and the argument against having this for any other numbers is that people would use it in weird ways. But people will use anything in weird ways, there's no stopping it. So it's best IMO to add support together with some guidelines for proper use.

Short of that, I understand these issues are very tough to work through. And it's not pressing anyway, I can just stay with 0.6 until there's a solution. For now it's more important for you to get some sleep :)

@Tokazama
Copy link
Collaborator

Is this foo(::Real) function defined outside of base? Maybe recommend less strict types in that case?

I'm a little baffled that StaticInt hasn't been readily accepted into Base. I can see how the other types in this package are harder to convince people of because it seems like a niche use case at first. However, once you get used to the mindset it really does eliminate the need for nearly any other situation with a Val{T} like parameter because you just use one of these static types as a field that can optionally be dynamic. At that point we are preventing people from developing the bad habit of creating new static parameters, which seems like a language feature (not a package).

@cscherrer
Copy link
Contributor Author

Is this foo(::Real) function defined outside of base?

Yes, the current case I'm hitting is

MethodError: no method matching exp(::Type{LogarithmicNumbers.ULogarithmic}, ::Static.StaticFloat64{0.0})

But I think constraining an argument to be <:Real is pretty common, so a PR to change this case would likely just leave me hitting the next case down the line.

I'm a little baffled that StaticInt hasn't been readily accepted into Base.

Me too. Some devs seem pretty type-averse in general, which is kind of foreign to me. I think it was a mistake to have Array not statically-sized by default, and this could be a great first step toward fixing that.

@Tokazama
Copy link
Collaborator

Is this foo(::Real) function defined outside of base?

Yes, the current case I'm hitting is

MethodError: no method matching exp(::Type{LogarithmicNumbers.ULogarithmic}, ::Static.StaticFloat64{0.0})

But I think constraining an argument to be <:Real is pretty common, so a PR to change this case would likely just leave me hitting the next case down the line.

Couldn't it be argued that using a subtype of Real in such cases is excessively restrictive? Is there any clear benefit for the restriction? If not, then it should be changed independent of anything going on here.

If there is a practical reason for the restriction then I'll have to think about it more. There may not be an easy solution.

I'm a little baffled that StaticInt hasn't been readily accepted into Base.

Me too. Some devs seem pretty type-averse in general, which is kind of foreign to me. I think it was a mistake to have Array not statically-sized by default, and this could be a great first step toward fixing that.

I get why things are the way they are. We had to start somewhere. But it feels like I'd have to rewrite a bunch of internal stuff in base to make it clear how useful this paradigm shift would be to some, and that sort of wide-sweeping change is not really how we're supposed to make collaborative improvements in software development.

@devmotion
Copy link
Member

We already have type-level support for irrationals that seems to work very well.

Slightly unrelated, but IMO the situation with irrationals is far from optimal. Defining them outside of base is always type piracy and was not intended by the original authors (as far as I understand). Additionally, promotion is a mess and can lead to surprising/undesired results.

@cscherrer
Copy link
Contributor Author

Couldn't it be argued that using a subtype of Real in such cases is excessively restrictive?

I think people often do this when something won't work for complex values. As I understand, best practice is to use type constraints for dispatch and not for restricting the domain of a function, but you still see it all over the place.

We had to start somewhere.

Yes of course, and it's also easy to forgive occasional weirdness like this when so many other design choices are really solid.

But it feels like I'd have to rewrite a bunch of internal stuff in base to make it clear how useful this paradigm shift would be to some, and that sort of wide-sweeping change is not really how we're supposed to make collaborative improvements in software development.

Could there be a smaller version of this? Outside of Base, ignoring load times, ignoring type piracy concerns, not as part of a package but just to demonstrate what's possible?

Defining them outside of base is always type piracy and was not intended by the original authors (as far as I understand). Additionally, promotion is a mess and can lead to surprising/undesired results.

Thanks David. I'd never thought about defining new irrationals - you're right that it's type piracy, because it defines a Base function on Symbol. Maybe there could be a way for that to take a module as its first argument?

The promotion issue is less clear to me.

@devmotion
Copy link
Member

The promotion issue is less clear to me.

I was thinking about behaviour such as the one described just yesterday on Slack (https://julialang.slack.com/archives/C67910KEH/p1655450699891149):

julia> typeof(-pi*1.0f0)
Float64

julia> typeof(pi*1.0f0)
Float32

julia> typeof(-pi)
Float64

@cscherrer
Copy link
Contributor Author

Good example, ouch

@Tokazama
Copy link
Collaborator

@cscherrer, is this still very problematic for you? We discussed what it might look like to pull out StaticFloat64 into its own package. Do we need to try that?

@cscherrer
Copy link
Contributor Author

Thanks @Tokazama . Yes, we currently can't upgrade to 0.7 because so many functions in other packages want arguments <:Real. I'm not sure about the StaticInt side... do things like array indexing want <:Integer, or is <:Number enough? If Number is ok for I think we could have a separate StaticFloats.jl and then just use StaticInt from 0.7 here.

@Tokazama
Copy link
Collaborator

I'm not sure about the StaticInt side... do things like array indexing want <:Integer, or is <:Number enough?

The move to Number works because indexing in base doesn't work with Number. A couple random places have Real or there's some weird promotion thing with Real and Integer that complicates invalidations. As far as I can tell JuliaLang/julia#44538 is at a stand still. Meaningful decisions related to StaticInt are a bit difficult to make until we have more info related to that one. If we can disentangle StaticFloat64's implementation from StaticInt, then it might allow more movement.

@chriselrod
Copy link
Collaborator

Is indexing arrays with StaticInt something you need for your distributional transformations?
If not, you should have your DSL's codegen auto-convert them to Int.

@cscherrer
Copy link
Contributor Author

Currently no. I guess it's not really indexing - more about representing array sizes. Static sizing doesn't seem very clean yet, for example something like a Cholesky factorization can end up with the size information buried deep in the types. So it's still not clear to me yet what the interactions look like between Static.jl, StaticArrays.jl, and ArrayInterface.jl.

Anyway, that's really more of a side question. If 0.7's approach to StaticInts works well for you it will probably be fine for me too.

@Tokazama
Copy link
Collaborator

If you're confident that it would be helpful, then I'd be happy to review/assist in getting something like StaticFloat.jl going (I can't personally take that one on at this time). I mostly want to make sure that your concerns were addressed after https://discourse.julialang.org/t/static-jl-vs-staticnumbers-jl/87228.

@cscherrer
Copy link
Contributor Author

Thanks @Tokazama . I don't have a sense if there might be a lot more to this than I can see, but I'll take a stab at it. It will be https://github.com/cscherrer/StaticFloats.jl - I'm modifying a fork now so we can keep the Git history as accurate as possible.

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 a pull request may close this issue.

4 participants