Harmonize code and improve docs #23
Conversation
|
I think I might also have broken compatibility with 1.0 by splitting test, docs and main dependencies using separate |
|
Yeah, that’s post-1.0. |
Is it bad? I don't realize how many people are still stuck with 1.0. @mschauer would you like to review? |
src/metagraph.jl
Outdated
| } <: AbstractGraph{T} | ||
| graph::Graph | ||
| vprops::Dict{Label,Tuple{T,VertexMeta}} | ||
| labels::Dict{T,Label} |
There was a problem hiding this comment.
I was never much of a fan of these fieldnames. I kept them to match with the original MetaGraphs. I think abbreviations make code hard to read, so I would have chosen something like vertex_codes and vertex_properties. If we are changing the fieldnames, though, we might want to release a new minor version, because we documented them and someone might be relying on them.
There was a problem hiding this comment.
I agree with you on this. In fact, that is the convention I chose for my own personal twist on MetaGraphsNext, DataGraphs. I'll change it here then.
In any case, I think a PR of this size warrants a minor version release
There was a problem hiding this comment.
Oh, maybe I misread. Why are you splitting up the label -> (code, metadata) Dict? Seems like that could lead to extra allocations?
|
Looks good to me (pending fixing CI)! Writing isn't my strong suit, so I'm glad you're working on it. |
Regarding the CI fix:
|
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
===========================================
- Coverage 100.00% 91.96% -8.04%
===========================================
Files 8 7 -1
Lines 230 224 -6
===========================================
- Hits 230 206 -24
- Misses 0 18 +18
Continue to review full report at Codecov.
|
|
I think it's probably a good idea to keep 1.0 compatibility unless it becomes unmanageable, just in case. |
Yes, I went back on that :) |
|
I only have to rename the fields and I think I'll be done |
|
I'm still a bit concerned about this #23 (comment) |
Where do you fear extra allocations? |
|
I mean, I'm not 100% sure, but I suspect 2 dicts rather than 1 will lead to extra allocations? |
|
It is indeed very possible, but I think the gain in clarity may be worth it 🤷 |
|
OK @bramtayl I reverted the changes I made and went back to one single |
|
Sure |
Hey there,
I spent some time improving code quality in the package, standardizing notations, adding docstrings, that kind of things. I would love some feedback on this, especially since I seem to have broken the doctests on saving graphs to file. Since IO is not my strong suit, contributions are welcome!