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.

Sign up for GitHub

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

Jump to bottom

Infinite recursion in wrapped arrays #145

Closed
wants to merge 3 commits into from
Closed

Infinite recursion in wrapped arrays #145

wants to merge 3 commits into from

Conversation

mzgubic
Copy link
Member

@mzgubic mzgubic commented Feb 8, 2021

Closes #141

mzgubic
mzgubic commented Feb 8, 2021
View reviewed changes
test/to_vec.jl
@@ -18,6 +18,9 @@ struct FillVector <: AbstractVector{Float64}
len::Int
end

Base.size(x::FillVector) = (x.len,)
Copy link
Member Author

Choose a reason for hiding this comment

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

This has just been moved

@mzgubic
Copy link
Member Author

mzgubic commented Feb 8, 2021

Do we care about Julia nightly failed tests?

@willtebbutt
Copy link
Member

willtebbutt commented Feb 8, 2021 •
edited
Loading

Do we care about Julia nightly failed tests?

I don't think so. @nickrobinson251 any thoughts?

I wonder whether this is a good way to solve this problem. In particular, it requires that whichever AbstractArray is to_veced define convert from Array to whatever type it is.

Tackling the BlockDiagonal example directly, do we have enough information in the type to know how to convert (required in from_vec) a Matrix{<:Real} to a BlockDiagonal? Is it not under-specified, since we don't know the block sizes in convert?

nickrobinson251
nickrobinson251 approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

LGTM

Tests on Nightly version of Julia don't matter (they look entirely unrelated to this PR)

src/to_vec.jl
@@ -54,7 +54,7 @@ function to_vec(x::AbstractVector)
end

function to_vec(x::AbstractArray)
x_vec, from_vec = to_vec(vec(x))
x_vec, from_vec = to_vec(collect(vec(x)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
x_vec, from_vec = to_vec(collect(vec(x)))
# `collect` to avoid recursion in wrapped arrays
x_vec, from_vec = to_vec(collect(vec(x)))

@mzgubic
Copy link
Member Author

mzgubic commented Feb 9, 2021

Having slept on it it's probably best to not merge this and just define a to_vec for array types directly for now. I think the current error is easier to debug than what we would get from this.

The long term plan is to get rid of to_vec anyway in favour of ChainRules differential types anyway right?

@willtebbutt
Copy link
Member

The long term plan is to get rid of to_vec anyway in favour of ChainRules differential types anyway right?

It is indeed the plan.

@mzgubic mzgubic closed this Apr 20, 2021
@gdalle
Copy link
Member

gdalle commented Mar 31, 2024

Can I suggest revisiting this PR? I found it was needed to make DifferentiationInterface.jl work with FiniteDifferences.jl, see #141 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickrobinson251 nickrobinson251 nickrobinson251 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

infinite recursion of to_vec for wrapped arrays
4 participants
@mzgubic @willtebbutt @gdalle @nickrobinson251