Modify condition in MultiDimClosure function#35
Open
Zinoex wants to merge 4 commits intoJuliaServices:mainfrom
Open
Modify condition in MultiDimClosure function#35Zinoex wants to merge 4 commits intoJuliaServices:mainfrom
Zinoex wants to merge 4 commits intoJuliaServices:mainfrom
Conversation
Member
|
Thanks for the fix! The change looks correct - without this condition, loading jagged arrays like Could you add a test case that covers this scenario? Something like: # Test jagged nested arrays (Array of Vectors)
jagged = [[1, 2], [3, 4, 5], [6]]
result = StructUtils.make(Vector{Vector{Int}}, jagged)
@test result == jagged
# Or a multi-dimensional case
md_jagged = Array{Vector{Int}}(undef, 2, 2)
md_jagged[1,1] = [1, 2]
md_jagged[1,2] = [3]
md_jagged[2,1] = [4, 5, 6]
md_jagged[2,2] = [7, 8]
# test round-trip through makeOnce a test is added, this looks good to merge! |
Author
|
@quinnj Adding the tests revealed that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For context, I am trying to load a
Array{Vector{Int64}, 4}using JSON.jl, which relies on StructUtils.jl. The inner vector is jagged, i.e. not the same length for every element, soArray{Int64, 5}is not an option. Now, without this stop condition, the multidimensional array continues untilf.cur_dim[] == 0and fails to set the dimension, unsurprisingly. With this condition it works flawlessly.Full disclaimer, I don't know if this will have a major impact elsewhere, so please review this PR carefully. But it would be nice if loading vectors in multi-dim arrays worked.