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

Make duals' operations through steprange use linrange instead #618

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tom-plaa
Copy link

Trying to solve #380.
Does not fix the "show" ambiguity.
A bit naively done, please review and advise if there's anything that needs to be done differently.

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on!

I had a go at writing some tests...

src/dual.jl Outdated Show resolved Hide resolved
test/DualTest.jl Outdated Show resolved Hide resolved
Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>
@mcabbott
Copy link
Member

Some other ways to make StepRangeLen with Dual, one of which makes a TwicePrecision{Dual}:

julia> (1:4) * Dual(1,2) |> typeof  # UnitRange, not tested
StepRangeLen{Dual{Nothing, Int64, 1}, Dual{Nothing, Int64, 1}, Dual{Nothing, Int64, 1}, Int64}

julia> (1:2:4) * Dual(1.0,2) |> typeof
StepRangeLen{Dual{Nothing, Float64, 1}, Dual{Nothing, Float64, 1}, Dual{Nothing, Float64, 1}, Int64}

julia> (0:0.1:1) ./ Dual(1.0,2) |> typeof
StepRangeLen{Dual{Nothing, Float64, 1}, Base.TwicePrecision{Dual{Nothing, Float64, 1}}, Base.TwicePrecision{Dual{Nothing, Float64, 1}}, Int64}

These don't seem to cause problems, so perhaps it's simplest to leave them alone.

@mcabbott
Copy link
Member

Failure on 1.6 seems to be this, arguably a bug in Base, but can it be worked around here?

julia> using ForwardDiff: Dual

julia> range(1, step = Dual(1,1), length = 5)
ERROR: InexactError: Int(Int64, Dual{Nothing}(5,4))
Stacktrace:
 [1] Int64
   @ ~/.julia/packages/ForwardDiff/pDtsf/src/dual.jl:362 [inlined]
 [2] convert
   @ ./number.jl:7 [inlined]
 [3] _rangestyle(#unused#::Base.Ordered, #unused#::Base.ArithmeticWraps, a::Int64, step::Dual{Nothing, Int64, 1}, len::Int64)
   @ Base ./range.jl:118
...

julia> VERSION
v"1.6.0"

julia> range(1.0, step = Dual(1,1), length = 5)
ERROR: StackOverflowError:
Stacktrace:
 [1] _range(a::Float64, st::Dual{Nothing, Float64, 1}, #unused#::Nothing, len::Int64) (repeats 79984 times)
   @ Base ./range.jl:111

@tom-plaa
Copy link
Author

tom-plaa commented Feb 23, 2023

On 1.8, it seems the behavior is

julia> collect(range(1,1,length=5))
5-element Vector{Float64}:
 1.0
 1.0
 1.0
 1.0
 1.0

julia> collect(range(1, Dual(1,1), length=5))
5-element Vector{Dual{Nothing, Float64, 1}}:
 Dual{Nothing}(1.0,0.0)
 Dual{Nothing}(1.0,0.25)
 Dual{Nothing}(1.0,0.5)
 Dual{Nothing}(1.0,0.75)
 Dual{Nothing}(1.0,1.0)

Which operation is causing the errors? The one on the dual component? For the Int one, we could force it to convert to Float, I guess, since it seems that range on Base also does this (at least in this anecdotal example).

@lupemba
Copy link

lupemba commented Nov 28, 2023

I just ran into this issue. Is the PR still active?

@tom-plaa
Copy link
Author

tom-plaa commented Nov 30, 2023

If there's anything that needs to be changed for the PR to merged, I don't know exactly what is. Or if it should be redone.
@lupemba , if in the meantime it's a blocking issue for you, you can do some type piracy in your script by including the changes that are here. I did that in a project I have and it solved my problem.

@lupemba
Copy link

lupemba commented Dec 4, 2023

@tom-plaa,
Thanks for the advice. I have already solved the problem by just converting my small range to a vector.

I believe that the PR was not merge because of the failing tests on Julia 1.6. The CI tests are now so old that I cannot see the logs anymore. I therefore don't know which test that failed.
The PR is probably also a bit behind master.

I would suggest.

  • Pull the latest changes from master into the branch.
  • Use julia up to run the tests with julia 1.6 on you own computer.
  • Fix the failed test.
  • Update this PR
  • All checks should now be green.

I think they will merge this PR as when all the CI checks pass :)

@lupemba
Copy link

lupemba commented Dec 11, 2023

@tom-plaa
I might try to make a PR that solves this myself.
I would.

  • Copy the tests from this PR (Hope it is fine with you)
  • Try to add dual support directly for TwinPrecision .

@tom-plaa
Copy link
Author

Feel free to reuse my code. The tests were mostly written by @mcabbott, not myself.

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.

None yet

3 participants