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

Treerender proposal for a sorts of dictionaries and arrays #1927

Merged

Conversation

MarkusSchildhauer
Copy link

Solves the conflict of interests arising in #1926 and discussed in #1926 (comment).

All sorts of dictionaries and arrays are rendered primarily displaying their key/index => values. But additionally the properties of the underlying objects are browsable via a top list entry called #properties (using any-icon and the type display style, but that can be changed easily). I recommend leaving the properties entry on top to make it more acceptable for use cases where browsing the properties is more likely (e.g. ODESolutions) and because it could be seen as some sort of meta information.

tree-dict-array-proposal

else
treerender(LazyTree(string(typeof(x)), wsicon(x), function ()
collect([SubTree(string(f), wsicon(getfield_safe(x, f)), getfield_safe(x, f)) for f in fields])
Copy link
Author

@MarkusSchildhauer MarkusSchildhauer Feb 11, 2021

Choose a reason for hiding this comment

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

I removed "collect" because it makes a shallow copy of an already allocated array that seems to be of no use here.

Copy link
Member

Choose a reason for hiding this comment

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

We do actually need the collects here:

julia> x = @SArray [1,2,3]
3-element SArray{Tuple{3},Int64,1,3} with indices SOneTo(3):
 1
 2
 3

julia> [x^2 for (i,x) in zip(keys(x), vec(x))]
3-element SizedArray{Tuple{3},Int64,1,1} with indices SOneTo(3):
 1
 4
 9

julia> collect([x^2 for (i,x) in zip(keys(x), vec(x))])
3-element Array{Int64,1}:
 1
 4
 9

I've added a few comments about why it's necessary.

Copy link
Author

Choose a reason for hiding this comment

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

OMG😯! I was not aware of that occasional iterator type dependency of comprehensions. But makes sense if special indexing is involved.

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

Love this!
I've added two more commits that fix various minor issues (see inline comments). Could also use a CHANGELOG entry, but I can do that later.

@@ -126,7 +152,8 @@ struct Undef end
const UNDEF = Undef()

function assign_undefs(xs)
xs′ = similar(xs, Any)
s = size(xs)
xs′ = Array{Any,length(s)}(undef, s...)
Copy link
Member

Choose a reason for hiding this comment

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

similar fails for readonly arrays, so this makes sure we use an Array.

Copy link
Member

Choose a reason for hiding this comment

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

Or rather, similar of a read-only array produces another read-only array, which then causes a failure on the setindex! call below.

end

function treerender(err, bt)
function treerender(err::Exception, bt)
Copy link
Member

Choose a reason for hiding this comment

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

Needs the ::Exception because otherwise it overwrites a method definition above.

# but we only want Arrays here
collect([SubTree(repr(k), wsicon(v), v) for (k, v) in zip(keys(x), vec(assign_undefs(x)))])
end
x isa Array ? out : pushfirst!(out, SubTree("", wsicon(x), PropertyBox(x)))
Copy link
Member

Choose a reason for hiding this comment

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

No need to show an empty #properties item for Base.Arrays, so let's special case that here.

@MarkusSchildhauer
Copy link
Author

Great you approved! Thanks

@davidanthoff
Copy link
Member

@MarkusSchildhauer Thanks a lot of this PR! Just a note on the process: we are right now in bug-fix mode, so at the moment we are not merging features. But in a few days we'll open up master for features again, and then we'll merge this PR right away.

@MarkusSchildhauer
Copy link
Author

Nice. But I have it anyway 😉

@pfitzseb pfitzseb removed the request for review from a team March 26, 2021 12:12
@davidanthoff davidanthoff merged commit 824575b into julia-vscode:master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants