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

Shorten MetaGraph representation #64

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Conversation

huguesmp
Copy link
Contributor

This is an attempt to address #63.

It is also my first PR, and I'm not very experienfed with Julia, so any advice is welcome :)

@henrik-wolf
Copy link

Thank you for preparing this. I believe in this case it does not really matter, but it might be cleaner to get the type of the graph similar to how we get the type of the Label, VertexData... like this:

function Base.show(
    io::IO, meta_graph::MetaGraph{<:Any, BaseGraph, Label, VertexData, EdgeData}
) where {BaseGraph, Label, VertexData, EdgeData}
    print(...)
end

some documentation for this pattern

Secondly, the problem with printing a lot still persists when I put something really big as the label:

julia> mg = MetaGraph(g1, [i=>i for i in 1:100], [(src(e), dst(e))=>1 for e in edges(g1)], g1)

But I think we can just say that is not the intended use?

@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #64 (ad09111) into master (5c98f33) will not change coverage.
The diff coverage is n/a.

❗ Current head ad09111 differs from pull request most recent head 407a3a2. Consider uploading reports for the commit 407a3a2 to get more accurate results

@@           Coverage Diff           @@
##           master      #64   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files           7        7           
  Lines         302      302           
=======================================
  Hits          282      282           
  Misses         20       20           
Files Changed Coverage Δ
src/metagraph.jl 90.24% <ø> (ø)

@huguesmp
Copy link
Contributor Author

I changed the code to follow the pattern you describe (of which I was unaware, thank you :), it is indeed much cleaner and more in line with the rest of the code.

As for the graph data question, I always figured it was only meant for a title for the graph (that's what I've been using it for), but it's true that there is no restriction on what one can put there.

Maybe someone can chime in on what the intention is with that field.

@gdalle
Copy link
Member

gdalle commented Aug 14, 2023

I agree that graph data is not meant to scale with $|V|$ or $|E|$, otherwise it would be in the vertex and edge data. So I think this fix is fine, thank you!

@gdalle gdalle merged commit b769719 into JuliaGraphs:master Aug 14, 2023
4 checks passed
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