Skip to content

Fix to_vec with Integers #191

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

Closed
wants to merge 2 commits into from
Closed

Fix to_vec with Integers #191

wants to merge 2 commits into from

Conversation

willtebbutt
Copy link
Member

Integers aren't considered differentiable, and they're certainly not perturbable. This bug fix enforces that.

This will probably break some things downstream that depend on this bug.

@willtebbutt willtebbutt requested a review from mzgubic September 27, 2021 11:52
@oxinabox
Copy link
Member

What was the previous behavour?

@mzgubic
Copy link
Member

mzgubic commented Sep 27, 2021

What was the previous behavour?

I think previously it just treated the integers as embedded in the reals and returned a vector of length 1 with integer transformed to a real.

This will probably break some things downstream that depend on this bug.

We might want to add tests for this as well, like in:
https://github.com/JuliaDiff/FiniteDifferences.jl/pull/189/files

Also, the _int2zero in that PR was to get around some really annoying behaviour where j'vp is not computed properly. The reason is the following situation:

julia> v1, b1 = to_vec(6)

julia> v2, b2 = to_vec(6)

julia> res = b1(v1 - v2) # b1 will just return 6 whatever the vector is
6

_int2zero didn't feel like the right solution at the time, and it still doesn't. Maybe there is a better way around this

@willtebbutt
Copy link
Member Author

I think previously it just treated the integers as embedded in the reals and returned a vector of length 1 with integer transformed to a real.

Exactly.

We might want to add tests for this as well, like in:
https://github.com/JuliaDiff/FiniteDifferences.jl/pull/189/files

Lol sorry, I totally forgot about that PR. I'll close this PR in favour of that.

@mzgubic
Copy link
Member

mzgubic commented Sep 27, 2021

I am not working on it actively, feel free to take it over!

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.

3 participants