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

Add rmul! #546

Merged
merged 1 commit into from
May 18, 2018
Merged

Add rmul! #546

merged 1 commit into from
May 18, 2018

Conversation

ararslan
Copy link
Member

Fixes #496.

@ararslan ararslan requested a review from martinholters May 15, 2018 22:21
@fredrikekre
Copy link
Member

This is more involved than just const rmul! = Base.scale!, see JuliaLang/julia#25701 (and JuliaLang/julia#25812).

README.md Outdated
@@ -400,6 +400,8 @@ Currently, the `@compat` macro supports the following syntaxes:

* `LinSpace` is now `LinRange` ([#25896]).

* `scale!` is now `rmul!` ([#25571]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most importantly scale!(::AbstractArray, ::Number) is, but In general, it can be rmul! or lmul!.

src/Compat.jl Outdated
@@ -1842,6 +1842,13 @@ else
using LinearAlgebra: qr
end

if VERSION < v"0.7.0-DEV.3449"
const rmul! = Base.scale!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rather define a new function with only those methods where scale! does the right thing. Maybe only rmul!(::AbstractArray, ::Number) for now.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from narrowing the signature, it would also be nice if this lived in the LinearAlgebra module, so that e.g. using Compat.LinearAlgebra (without using Compat) makes it available, as it does on 0.7.

README.md Outdated
@@ -400,6 +400,8 @@ Currently, the `@compat` macro supports the following syntaxes:

* `LinSpace` is now `LinRange` ([#25896]).

* `scale!` is now `rmul!` ([#25571]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why #25571? Shouldn't that rather be #25701 and #25812?

@ararslan
Copy link
Member Author

it would also be nice if this lived in the LinearAlgebra module

Agreed. If you have any suggestions for how to structure that sensibly, do let me know; I wasn't able to think of a good solution.

src/Compat.jl Outdated
@@ -1842,6 +1842,13 @@ else
using LinearAlgebra: qr
end

if VERSION < v"0.7.0-DEV.3449"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong version? Should be 0.7.0-DEV.3563, but also needs a definition until 0.7.0-DEV.3665 in terms of mul1!.

@martinholters
Copy link
Member

If you have any suggestions for how to structure that sensibly, do let me know

I guess evaling the new definition into LinearAlgebra might be ok; it's not that much worse than the type-piracy that Compat commits all over the place. Otherwise, I'd favor simply not exporting rmul!, so that users need to write Compat.rmul!.

@martinholters
Copy link
Member

Or something along those lines might work:

module _LinearAlgebra
    using LinearAlgebra # or Base.LinAlg prior to 0.7.0-DEV.3449
    for n in names(LinearAlgebra)
        @eval export $n
    end
    # more definitions like rmul! as needed
end
const LinearAlgebra = _LinearAlgebra

@rofinn
Copy link
Contributor

rofinn commented May 17, 2018

FWIW, I'd also be fine with not exporting rmul!.

@ararslan
Copy link
Member Author

Yeah that seems like the way to go. evaling into a Base module seems pretty un-legit, which is why I didn't do that to begin with.

@ararslan
Copy link
Member Author

I redid this completely. Let me know how it looks now.

Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now.

@ararslan
Copy link
Member Author

Thanks Martin. @fredrikekre, did you want to take another look as well?

@ararslan
Copy link
Member Author

This is needed for progress elsewhere so I'm just going to go ahead and merge and tag.

@ararslan ararslan merged commit 7e88c20 into master May 18, 2018
@ararslan ararslan deleted the aa/rmul branch May 18, 2018 20:52
martinholters added a commit that referenced this pull request Oct 8, 2019
martinholters added a commit that referenced this pull request Oct 8, 2019
martinholters added a commit that referenced this pull request Oct 8, 2019
martinholters added a commit that referenced this pull request Oct 8, 2019
martinholters added a commit that referenced this pull request Oct 13, 2019
* Bump required Julia version to 1.0

* Remove compatibility support code for:
  * `at-__MODULE__` (from #363)
  * `devnull`, `stdin`, `stdout`, and `stderr` from #499
  * `at-nospecialize` (from #385 and #409)
  * `isabstracttype` and `isconcretetype` (from #477)
  * `invokelatest` from #424
  * array-like access to `Cmd` from #379
  * `Val(n)` and `ntuple`/`reshape` with `Val` from #381 and #399
  * `logdet(::Any)` fallback from #382
  * `chol(::UniformScaling)` from #382
  * `pushfirst!`, `popfirst!` from #444
  * `fieldcount` from #386
  * `read(obj, ::Type{String})` from #385 and #580
  * `InexactError`, `DomainError`, and `OverflowError` constructors from #393
  * `corrected` kw arg to `cov` from #401
  * `adjoint` from #401
  * `partialsort` from #401
  * `pairs` from #428
  * `AbstractRange` from #400
  * `rtoldefault` from #401
  * `Dates.Period` rounding from #462
  * `IterativeEigensolvers` from #435
  * `occursin` from #520
  * `Char` concatenation from #406
  * `BitSet` from #407
  * `diagm` and `spdiagm` with pairs from #408
  * `Array` c'tors from `UniformScaling` from #412 and #438
  * `IOContext` ctor taking pairs from #427
  * `undef` from #417 and #514
  * `get` on `ENV` from #430
  * `ComplexF...` from #431
  * `tr` from #614
  * `textwidth` from #644
  * `isnumeric` from #543
  * `AbstractDict` from #435
  * `axes` #435 and #442
  * `Nothing` and `Cvoid` from #435
  * `Compat.SuiteSparse` from #435
  * `invpermute!` from #445
  * `replace` with a pair from #445
  * `copyto!` from #448
  * `contains` from #452
  * `CartesianIndices` and `LinearIndices` from #446, #455, and #524
  * `findall` from #466 (and #467).
  * `argmin` and `argmax` from #470, #472, and #622
  * `parentmodule` from #461
  * `codeunits` from #474
  * `nameof` from #471
  * `GC` from #477
  * `AbstractDisplay` from #482
  * `bytesavailable` from #483
  * `firstindex` and `lastindex` from #480 and #494
  * `printstyled` from #481
  * `hasmethod` from #486
  * `objectid` from #486
  * `Compat.find*` from #484 and #513
  * `repr` and `showable` from #497
  * `Compat.names` from #493 and #505
  * `Compat.round` and friends #500, #530, and #537
  * `IOBuffer` from #501 and #504
  * `range` with kw args and `LinRange` from #511
  * `cp` and `mv` from #512
  * `indexin` from #515
  * `isuppercase` and friends from #516
  * `dims` and `init` kwargs from #518, #528, #590, #592, and #613
  * `selectdim` from #522 and #531
  * `repeat` from #625
  * `fetch(::Task)` from #549
  * `isletter` from #542
  * `isbitstype` from #560
  * `at-cfunction` from #553 and #566
  * `codeunit` and `thisind` and friends from #573
  * `something` from #562
  * `permutedims` from #582
  * `atan` from #574
  * `split` and `rsplit` from #572
  * `mapslices` from #588
  * `floatmin` and `floatmax` from #607
  * `dropdims` from #618
  * required keyword arguments from #586
  * `CartesianRange` in `at-compat` from #377
  * `finalizer` from #416
  * `readline`, `eachline`, and `readuntil` from #477, #541, and #575
  * curried `isequal`, `==`, and `in` from #517
  * `Some` from #435 and #563
  * `at-warn` and friends from #458

* Remove old deprecations

* Deprecate:
  * `Compat.Sockets` from #545 and #594
  * `TypeUtils` from #304
  * `macros_have_sourceloc` from #355
  * `Compat.Sys` from #380, #433, and #552
  * `Compat.MathConstants` from #401
  * `Compat.Test`, `Compat.SharedArrays`, `Compat.Mmap`, and `Compat.DelimitedFiles` from #404
  * `Compat.Dates` from #413
  * `Compat.Libdl` from #465 (and #467)
  * `AbstractDateTime` from #443
  * `Compat.Printf` from #435
  * `Compat.LinearAlgebra` from #463
  * `Compat.SparseArrays` from #459
  * `Compat.Random` from #460, #601, and #647
  * `Compat.Markdown` from #492
  * `Compat.REPL` from #469
  * `Compat.Serialization` from #473
  * `Compat.Statistics` from #583
  * `Fix2` from #517
  * `Compat.Base64` from #418
  * `Compat.Unicode` from #432 and #507
  * `notnothing` from #435 and #563
  * `Compat.IteratorSize` and `Compat.IteratorEltype` from #451
  * `enable_debug(::Bool)` from #458
  * `Compat.Distributed` from #477
  * `Compat.Pkg` from #485
  * `Compat.InteractiveUtils` from #485
  * `Compat.LibGit2` from #487
  * `Compat.UUIDs` from #490
  * `Compat.qr` from #534
  * `Compat.rmul!` from #546
  * `Compat.norm` abd friends from #577

* Remove obsolete README entry, missed in #385

* Remove obsolete tests (e.g. missed in #372)

* Remove obsolete `VERSION` conditionals and some minor clean-up
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.

4 participants