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

Revert broadening of arithmetic Any methods (revert #44564 and #45320) #45489

Merged
merged 3 commits into from
Jul 25, 2022

Conversation

DilumAluthge
Copy link
Member

See #45463 for context.


Reverts #44564 (reverts cf1f717)
Reverts #45320 (reverts 990b1f3)
Fixes #45463

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl
@DilumAluthge DilumAluthge changed the title Revert #44564 and #45320 Revert broadening of arithmetic Any methods (Revert #44564 and #45320) May 28, 2022
@DilumAluthge DilumAluthge changed the title Revert broadening of arithmetic Any methods (Revert #44564 and #45320) Revert broadening of arithmetic Any methods (revert #44564 and #45320) May 28, 2022
@dkarrasch
Copy link
Member

I thought the objections were against the (remaining) generic one- and two-arg --methods only. We could revert those changes by a "forward" PR similarly to #45320 and leave the / and \ related changes from #44564. The generic fallback for right solves required adjoint to be defined, which is undocumented, or even confusing. But I'm also fine with reverting the two PRs. In that case, it would be good to collect actionable things in an issue.

@dkarrasch
Copy link
Member

Hm, the / and \ related changes change the following behavior. On master:

julia> eltype(one(Int32) \ one(Float32))
Float64

julia> eltype(one(Int64) \ one(Float32))
Float64

On v1.7:

julia> eltype(one(Int32) \ one(Float32))
Float32

julia> eltype(one(Int64) \ one(Float32))
Float32

The reason is that the most generic \ method is called, which inverts the integer one and creates a Float64 that gets multiplied by the right Float32 number. At the same time, we cannot add

\(x::Number, y::Number) = \(promote(x,y)...)

because that causes a stack overflow since there is no float equivalent that catches the promoted call.

One might view that as another reason to merge this PR (and revert the others).

@KristofferC KristofferC added this to the 1.9 milestone Jun 30, 2022
@daanhb
Copy link
Contributor

daanhb commented Jul 24, 2022

I had a failing test in one of my packages and narrowed it down to a change in left division after #44564:

julia> a = BigFloat(1)
1.0

julia> 3 \ a
0.333333333333333314829616256247390992939472198486328125

There is loss of accuracy because left division started doing inv(3) * a. This might happen in more packages with high precision calculations.

daanhb referenced this pull request in JuliaApproximation/BasisFunctions.jl Jul 24, 2022
@dkarrasch
Copy link
Member

Ok, then it's clear that this here needs to be merged. I resolve the merge conflict and merge after tests pass, unless someone beats me to it.

One conclusion from this whole tour then is that we should be clearer in the docstrings for left-division about what the actual fallback is. The x \ y = inv(x) * y idea was taken from the docstring, which otherwise does not state that for custom types, you need to have right-division and adjoint (or conj for Number-subtypes) defined.

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label Jul 24, 2022
@daanhb
Copy link
Contributor

daanhb commented Jul 24, 2022

To be fair, I should be careful anyway about assuming that 3 \ a will give the right answer without converting 3 to the type of a in my case. I just wanted to point out that, as things stand, the change breaks some packages. I'll be changing my offending code regardless of the outcome here.

(In general, division might be safer than computing an inverse, but I did not think this through.)

@DilumAluthge
Copy link
Member Author

The Test i686-linux-gnu failures are #46159

@DilumAluthge DilumAluthge merged commit 783a6aa into master Jul 25, 2022
@DilumAluthge DilumAluthge deleted the dpa/revert-pr44564-and-pr45320 branch July 25, 2022 01:15
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jul 25, 2022
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
…and JuliaLang#45320) (JuliaLang#45489)

* Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)"

This reverts commit 990b1f3.

* Revert "Generalize or restrict a few basic operators (JuliaLang#44564)"

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl

Co-authored-by: Daniel Karrasch <[email protected]>
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
…and JuliaLang#45320) (JuliaLang#45489)

* Revert "Remove type-unlimited unary `+` and `*` (JuliaLang#45320)"

This reverts commit 990b1f3.

* Revert "Generalize or restrict a few basic operators (JuliaLang#44564)"

This reverts commit cf1f717.

Also fixes merge conflicts in stdlib/LinearAlgebra/test/generic.jl

Co-authored-by: Daniel Karrasch <[email protected]>
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.

revert broadening of arithmetic Any methods
4 participants