Type-stable constructors

[gdalle]

• Fix Add constructors that infer the metadata types from the data values provided to constructor #6 by adding type-stable constructors
• Fix Make keyword argument names consistent #27 by renaming keyword type arguments
• Fix Type stability tests #30 by adding inference tests
• Switch order of MetaGraph type parameters
• Remove Symbol as default vertex data type
• Tweak directedness traits to make arrange apply only to MetaGraphs
• Adapt copy to new constructors
• Implement full == comparing all fields
• Fix misleading documentation on weights
• Add labels function to iterate on vertex labels
• Stricter doc builds that error when something goes wrong are now part of CI for PRs

[codecov]

Codecov Report

Merging #45 (b14cae3) into master (fe2a5bb) will increase coverage by 2.84%.
The diff coverage is 97.87%.

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

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   90.83%   93.68%   +2.84%     
==========================================
  Files           7        7              
  Lines         240      269      +29     
==========================================
+ Hits          218      252      +34     
+ Misses         22       17       -5     

Impacted Files	Coverage Δ
src/MetaGraphsNext.jl	100.00% <ø> (ø)
src/dict_utils.jl	94.28% <ø> (ø)
src/metagraph.jl	95.12% <97.14%> (+10.50%)	⬆️
src/directedness.jl	100.00% <100.00%> (ø)
src/graphs.jl	86.86% <100.00%> (+3.37%)	⬆️
src/weights.jl	100.00% <100.00%> (+5.55%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[gdalle]

Sorry @bramtayl this is a hell of a PR to review but we're making progress!

[bramtayl]

Thanks!

• I'm not sure "type unstable" in the right terminology here. Maybe something like "the type of the result depends on the keyword arguments, so if you are going to construct multiple graphs, do it inside a function, so the keyword arguments can constant propagate".
• I'm not sure we need to export the positional constructor, although it's useful to have around. I like the vertices_description etc constructor though.
• Instead of @asserts, maybe we could throw more helpful errors? Why don't we set it up as a bounds check. After we check bounds, we can @inbounds everything. Then users can skip the bounds check if they want to play dangerous?
• As far as documenting the type stability, instead of just throwing in a bunch of @inferred, why don't we just have one or two examples comparing MetaGraphs with MetaGraphsNext? Everything else looks good!

[gdalle]

• I'm not sure "type unstable" in the right terminology here. Maybe something like "the type of the result depends on the keyword arguments, so if you are going to construct multiple graphs, do it inside a function, so the keyword arguments can constant propagate".

Thanks, clarified this in the last commit

[gdalle]

• I'm not sure we need to export the positional constructor, although it's useful to have around. I like the vertices_description etc constructor though.

As long as the positional constructor exists, it will be exported because it shares the name of the struct. It's not really a problem though cause it doesn't cause ambiguities

[gdalle]

• As far as documenting the type stability, instead of just throwing in a bunch of @inferred, why don't we just have one or two examples comparing MetaGraphs with MetaGraphsNext? Everything else looks good!

There is a slight subtlety about tihs: the @inferred tests do not show up in the docs, they are only used for testing. Since I'm building the Markdown pages from pure Julia files with Literate.jl, I can choose to filter out certain lines from the Markdown output. This is what the #src at the end of these lines is for.

If you want you can git checkout the PR and then build the docs locally to get an idea of the new look.

[gdalle]

• Instead of @asserts, maybe we could throw more helpful errors? Why don't we set it up as a bounds check. After we check bounds, we can @inbounds everything. Then users can skip the bounds check if they want to play dangerous?

In the latest commit I turned AssertionErrors into ArgumentErrors with a more helpful message. I'm not necessarily in favor of @inbounds everywhere, at least not without careful profiling. For instance my gut tells me we lose way more time in Dict access than in bounds checking anyway. I would leave it as is for now

[gdalle]

@bramtayl ready for round 2 and possible approval

[bramtayl]

This looks great to me thanks!