Closed gdalle closed 1 year ago
Merging #45 (b14cae3) into master (fe2a5bb) will increase coverage by
2.84%
. The diff coverage is97.87%
.:exclamation: 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%) |
:arrow_up: |
src/directedness.jl | 100.00% <100.00%> (ø) |
|
src/graphs.jl | 86.86% <100.00%> (+3.37%) |
:arrow_up: |
src/weights.jl | 100.00% <100.00%> (+5.55%) |
:arrow_up: |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Sorry @bramtayl this is a hell of a PR to review but we're making progress!
Thanks!
vertices_description
etc constructor though.@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?@inferred
, why don't we just have one or two examples comparing MetaGraphs
with MetaGraphsNext
? Everything else looks good!
- 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
- 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
- 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 comparingMetaGraphs
withMetaGraphsNext
? 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.
- 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 AssertionError
s into ArgumentError
s 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
@bramtayl ready for round 2 and possible approval
This looks great to me thanks!
MetaGraph
type parametersSymbol
as default vertex data typearrange
apply only toMetaGraphs
copy
to new constructors==
comparing all fieldslabels
function to iterate on vertex labels