ReverseView and UndirectedView#376
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 97.26% 97.30% +0.04%
==========================================
Files 122 123 +1
Lines 7340 7378 +38
==========================================
+ Hits 7139 7179 +40
+ Misses 201 199 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@gdalle Should be good for review. Do you know what's wrong with the documentation? |
| struct ReverseView{T<:Integer,G<:AbstractGraph} <: AbstractGraph{T} | ||
| g::G | ||
|
|
||
| @traitfn ReverseView{T,G}(g::::(IsDirected)) where {T<:Integer,G<:AbstractGraph{T}} = | ||
| new(g) | ||
| @traitfn ReverseView{T,G}(g::::(!IsDirected)) where {T<:Integer,G<:AbstractGraph{T}} = | ||
| throw(ArgumentError("Your graph needs to be directed")) | ||
| end |
There was a problem hiding this comment.
Wouldn't it make more sense if this function works on all graphs? Even if it does nothing for undirected graphs.
| 1 | ||
| ``` | ||
| """ | ||
| struct UndirectedView{T<:Integer,G<:AbstractGraph} <: AbstractGraph{T} |
There was a problem hiding this comment.
Could we distinguish between weak and strong connectivity? I.e. weaks means that v and w in the undirected view are connected if either v -> w or v <- w in the original graph. Strong means that v <-> w in the original graph.
| """ | ||
| struct UndirectedView{T<:Integer,G<:AbstractGraph} <: AbstractGraph{T} | ||
| g::G | ||
| ne::Int |
There was a problem hiding this comment.
Precalculating ne is a bit dangerous as one might change the underlying graph and then the view would be invalid. If we cache this value then we should be very clear about that in the documentation.
I think it might be worth not to cache it and then make sure that all our algorithms calculate ne only once.
| @traitfn function UndirectedView{T,G}( | ||
| g::::(IsDirected) | ||
| ) where {T<:Integer,G<:AbstractGraph{T}} | ||
| ne = count(e -> src(e) <= dst(e) || !has_edge(g, dst(e), src(e)), Graphs.edges(g)) |
There was a problem hiding this comment.
| ne = count(e -> src(e) <= dst(e) || !has_edge(g, dst(e), src(e)), Graphs.edges(g)) | |
| ne = count(e -> src(e) <= dst(e) && src(e) <= dst(e), Graphs.edges(g)) |
This way we can avoid the expensive call to has_edge!
There was a problem hiding this comment.
This seems to be checking the same thing twice?
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…underlying graph UndirectedView caches the number of edges at construction time, and ReverseView forwards properties from the underlying graph. In both cases, modifying the underlying graph after constructing the view can lead to incorrect results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
There are a number of functionality and performance improvements left suggested in the review, but all documentation and correctness comments are now addressed. I will proceed with merging this. Future PRs can address the missing features and improve performance. |
4 participants
No description provided.