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

docs: add section targeted towards people coming from MetaGraphs.jl #29

Closed
wants to merge 3 commits into from

Conversation

mroavi
Copy link

@mroavi mroavi commented Apr 28, 2022

Hi there. This is my initial attempt to tackle #26.
I basically translated all the examples in the MetaGraphs.jl Example Usage section and put them next to each other on a separate page of the docs.
I'm sure that there are better ways to do this since I'm new to MetaGraphsNext.jl. Looking forward to hearing about them. Let me know what you think.

@bramtayl
Copy link
Collaborator

Thanks for all this! It might take a bit of time to review but I think it's a great idea!


Using *MetaGraphsNext.jl*:
```@example main
mg = MetaGraphsNext.MetaGraph(Graph(), default_weight = 3.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of doing each vertex one at a time, it might be nice to add a constructor to do it all at once. Ref #6?

Copy link
Author

Choose a reason for hiding this comment

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

Yes! That constructor would be super cool to have. Perhaps I can find some time to implement it. I think I would need some assistance though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, let me know if you have any questions.

@bramtayl
Copy link
Collaborator

Also, in general, it would be good to format these all as doctests.


Using *MetaGraphsNext.jl*:
```@example main
# Not possible since but this is a workaround
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not necessarily. You could use a mutable struct or a dict as vertex metadata.


foreach(x -> add_vertex!(mg, Symbol(x), (name = "", id = missing)), 1:5)
foreach(x -> add_edge!(mg, Symbol(x[1]), Symbol(x[2]), ""), zip(1:5, 2:5))
mg[Symbol(1)] = (name = "Susan", id = 123)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems confusing. Why not just use a symbol like :index1 or something?


Using *MetaGraphsNext.jl*:
```@example main
# Not supported. The exact data type of the vertex data needs to be defined
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could have a dict with eltype Any as vertex metadata. Not sure that would be advisable, but it would work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in general, using dicts with eltype Any as vertex metadata might be the best way to show people how to migrate from MetaGraphs to MetaGraphsNext. Then, afterwards, you could talk about using something more strongly typed, like a NamedTuple or mutable struct, might be better if all the metadata has same structure.

@mroavi
Copy link
Author

mroavi commented Apr 29, 2022

Also, in general, it would be good to format these all as doctests.

All of them? Including the ones running MetaGraphs.jl examples? That means that we would need to add MetaGraphs.jl as a dependency for MetaGraphsNext.jl tests.

@bramtayl
Copy link
Collaborator

Nah, just the MetaGraphsNext ones. Does that make sense?

@mroavi
Copy link
Author

mroavi commented Apr 29, 2022

Nah, just the MetaGraphsNext ones. Does that make sense?

Yeap. Take a look at the last commit to make sure we are on the same page.


Using *MetaGraphsNext.jl*:
```jldoctest main
# Important: graph data can only be set when constructing the object
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is arguably a bug. We could make just this one field mutable on more recent versions of Julia.

@bramtayl
Copy link
Collaborator

bramtayl commented May 2, 2022

Thanks! The doctests look good as long as they pass CI.

@mroavi
Copy link
Author

mroavi commented May 19, 2022

Thanks! The doctests look good as long as they pass CI.

I removed the Combinatorics package as a dependency. I think the tests should pass now.

@gdalle
Copy link
Member

gdalle commented Mar 9, 2023

Hey @mroavi, and thank you for your contribution!
Since there was quite a big change in the constructors and documentation of MetaGraphsNext (see #45), I fear this PR may no longer be compatible with the current state of the package. Since there is also a (small) section on MetaGraphs in the current documentation, I would like to shift this PR to draft status, until we decide how best to proceed. Is that okay?

@mroavi
Copy link
Author

mroavi commented Mar 9, 2023

Sounds good!

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