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

zeros etc. is arguably broken for mutable element types #29168

Closed
jrevels opened this issue Sep 13, 2018 · 14 comments
Closed

zeros etc. is arguably broken for mutable element types #29168

jrevels opened this issue Sep 13, 2018 · 14 comments
Labels
domain:arrays [a, r, r, a, y, s]

Comments

@jrevels
Copy link
Member

jrevels commented Sep 13, 2018

Apologies if dup.

julia> mutable struct Foo
       end

julia> Base.zero(::Type{Foo}) = Foo()

julia> x = zeros(Foo, 2)
2-element Array{Foo,1}:
 Foo()
 Foo()

julia> x[1] === x[2]
true

I would've expected each element to have been instantiated separately via a new zero call. If I wanted this behavior, I would've just called fill(zero(Foo), 2).

see jump-dev/JuMP.jl#1456 (comment)
obligatory xref to #24595

@JeffBezanson
Copy link
Sponsor Member

This might be ok to change, but can it really be enough? Do we have to make a rule that you must never write z=zero(T); [z, z]?

This also has a performance cost. We don't want to allocate a new number for each element for BigInt, and there also might be other types where convert(T, 0) is expensive for some reason. In those cases we'd have to make sure to cache constants to return from zero and one.

@mbauman
Copy link
Sponsor Member

mbauman commented Sep 13, 2018

I would've expected each element to have been instantiated separately via a new zero call. If I wanted this behavior, I would've just called fill(zero(Foo), 2).

That's completely opposite my expectation here, but perhaps that because I've touched the implementation. I expect zeros to be a simple shorthand for fill.

@jrevels
Copy link
Member Author

jrevels commented Sep 13, 2018

Do we have to make a rule that you must never write z=zero(T); [z, z]?

If that's the implementation of fill(zero(T), 2), then that looks like a correct implementation to me. If that's the implementation of zeros(T, 2), that looks incorrect - I would've expected it to return the equivalent of [zero(T), zero(T)].

Now, I understand the zeros documentation doesn't currently explicitly require this behavior. This issue is arguing that it should at least define which behavior it's using - fill vs. new instance for every element. I personally think the latter is more useful since we already have fill.

We don't want to allocate a new number for each element for BigInt, and there also might be other types where convert(T, 0) is expensive for some reason. In those cases we'd have to make sure to cache constants to return from zero and one.

This is job of the caller, isn't it? If they want the same instance in every element, that's what fill is for. If they are requesting an array where each element is a new instance, then that's what they should receive. IMO, it's nicer for the caller when the called function isn't making this kind of decision for you for generic types. Maybe for BigInt it could be left in as an optimization but even that seems wrong to me/a "gotcha".

@jrevels
Copy link
Member Author

jrevels commented Sep 13, 2018

I expect zeros to be a simple shorthand for fill.

If the consensus ends up being that this is the expectation, and I'm just the odd one out, then we can just document it and move on.

@mbauman
Copy link
Sponsor Member

mbauman commented Sep 13, 2018

I agree it feels funny in the case of JuMP that you linked… but I don't think it'd be terribly offensive for JuMP to add a zeros(::JuMPVariable, …) that does what you want.

@mbauman mbauman added the domain:arrays [a, r, r, a, y, s] label Sep 13, 2018
@StefanKarpinski
Copy link
Sponsor Member

To me the term "zero" implies that something is numeric, which in turn implies that its identity should be determined by its value and it probably shouldn't be mutable. It's fine if it happens to be mutable but behave in a reasonable way for numbers (like BigInts, for example) but I think that being able to mutate zero is the real root of the problem here.

@jrevels
Copy link
Member Author

jrevels commented Sep 13, 2018

I don't think it'd be terribly offensive for JuMP to add a zeros(::JuMPVariable, …) that does what you want.

Yeah, JuMP already has the problem that it attempts to use scalars' types to dictate array behavior, which makes this annoying to implement without a (needed, IMO) refactor of the JuMP code, but that's besides the more general problem.

I think that being able to mutate zero is the real root of the problem here.

Agreed. Base's numeric interface in general has a lot of unstated assumptions like these.

@JeffBezanson
Copy link
Sponsor Member

If they are requesting an array where each element is a new instance, then that's what they should receive.

Nobody wants an array of BigInts that uses enormously more memory than it needs to.

I'll point out that during 0.7 development ones and zeros were almost deprecated to fill, so to many people they are equivalent. References:
#25392 #24444 #25087 #24656
#24444 (comment)

If that's the implementation of fill(zero(T), 2), then that looks like a correct implementation to me. If that's the implementation of zeros(T, 2), that looks incorrect

It's neither --- it's just random code somewhere, where somebody is making an array of two zeros. The point is that zeros is just one function, and not all similar code will necessarily go through that function.

Base's numeric interface in general has a lot of unstated assumptions like these.

The idea that zeros (and numbers generally) are immutable is not just some random unstated assumption. It is nearly impossible to do any mathematical or numerical programming with "mutable numbers" that always allocates and/or shares references as somebody expects.

If this behavior of zeros is just needed internally by JuMP then it's not a totally unreasonable request, but could also be replaced with a custom function. But if the goal is to feed a mutable T to generic code that might call zeros and have it do what you want, it will never work --- you'd have to get everybody to use zeros instead of fill, etc.

@jrevels
Copy link
Member Author

jrevels commented Sep 13, 2018

It's neither --- it's just random code somewhere, where somebody is making an array of two zeros.

You asked me whether I thought people should never write that code. My answer was "it depends on what the intention of the code is" and I gave examples that seemed reasonable to me.

not all similar code will necessarily go through that function.

I'm not arguing for that at all. Just that a function's behavior be well-defined in the documentation (with a preference towards a certain behavior, but deference towards the consensus opinion).

The idea that zeros (and numbers generally) are immutable is not just some random unstated assumption.

I never said it was random, I'm not even saying it's unreasonable. It is unstated. AFAIK (apologies if I'm wrong here), there's no documentation telling me that generic numeric code in Base relies on the assumption that my custom number types are immutable and/or don't maintain references to mutable state.

It is nearly impossible to do any mathematical or numerical programming with "mutable numbers" that always allocates and/or shares references as somebody expects.

This is quite incorrect. In other languages especially, but also in Julia, it is an extremely common way to implement various for numerical programming techniques, especially those that require graph accumulation/code interception. Obviously, I hope that new approaches using tools like Cassette will be better for such purposes, but this is the way things have been implemented in Julia for years now, JuMP being the obvious example.

If this behavior of zeros is just needed internally by JuMP then it's not a totally unreasonable request, but could also be replaced with a custom function.

We did write a custom function. We don't need to use zeros in JuMP. That's just where the issue was brought up originally.

I'm not trying to ruffle anybody's feathers, here. Apologies if I did.

@tkoolen
Copy link
Contributor

tkoolen commented Sep 13, 2018

Ref #27483, regarding fill/fill!.

@JeffBezanson
Copy link
Sponsor Member

My answer was "it depends on what the intention of the code is" and I gave examples that seemed reasonable to me.

But somebody who writes z=zero(T); [z, z] most likely doesn't have any intention about aliasing. Can you clarify whether there is a need to be able to inject mutable T into such code and have it work a certain way?

I can imagine that "mutable number"-based techniques might in fact work (though perhaps give slightly different answers) no matter how [z, z] is implemented (one object or two). If that's the case, then I agree there is no real problem with them. But if they live or die depending on whether the target code uses e.g. zeros or fill, then it doesn't seem practical to me.

@jrevels
Copy link
Member Author

jrevels commented Sep 14, 2018

If that's the case, then I agree there is no real problem with them.

That is generally the case, if the implementation is sound.

Just to clarify: this issue was not intended to be about the viability of zeros as a primitive for some transformation on a target program. That's a way more complex topic than this issue. Maybe it's highly related, though.

I was just pointing out that I thought I could use zeros(T, ...) as shorthand for calling zero(T) for every element of the returned array. The actual behavior was the fill behavior, though, which seemed counterintuitive (to me, since we already have fill), so I filed an issue.

@astrozot
Copy link

My two cents: I got quite confused in the past with the behavior of zeros with mutable types, since I expected what @jrevels says. I might be biased, because I am a Python programmer and therefore tend to associate Julia's zeros with the numpy equivalent function, but I feel I am in good company.

I see not much progress has been made on this side, but perhaps it is worth reconsidering this issue in the near future?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 31, 2023

I don't know how we would document that <: Number (and implementation of methods such as zero) implies that the object is not mutated, but that seems the intended expectation here. Some people use MutableArithmetic.jl, but arguably that would be completely fine if it did not try to extend the methods for AbstractMutable to also work with some Numbers (e.g. BigInt and BigFloat) and define them with zeros

Mostly just echoing Stefan's comment above #29168 (comment)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

7 participants