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

Fix could not import Graphs.prufer_decode warning #215

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

simonschoelly
Copy link
Contributor

There was an incorrect import statement for prufer_code in the SimpleGraphs submodule. This PR removes it.

@simonschoelly simonschoelly added the bug Something isn't working label Jan 14, 2023
@simonschoelly
Copy link
Contributor Author

@SimonCoste would you mind doing a quick review?

@codecov
Copy link

codecov bot commented Jan 14, 2023

Codecov Report

Merging #215 (228262d) into master (c4360cf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #215   +/-   ##
=======================================
  Coverage   97.43%   97.43%           
=======================================
  Files         113      113           
  Lines        6554     6554           
=======================================
  Hits         6386     6386           
  Misses        168      168           

@SimonCoste
Copy link
Contributor

SimonCoste commented Jan 20, 2023

How did you get this warning ?
Indeed I don't understand why the import statement is not correct. Shouldn't I import prufer_decode since I use it in the generator uniform_tree ?

@simonschoelly
Copy link
Contributor Author

Usually one does not notice, but when Graphs.jl gets precompiled there is a warning:

julia> using Graphs
[ Info: Precompiling Graphs [86223c79-3864-5bf0-83f7-82e725a168b6]
WARNING: could not import Graphs.prufer_decode into SimpleGraphs

I did't realize it, when I made this PR, but I think the reason is, that in src/Graphs.jl we first include src/SimpleGraphs/SimpleGraphs.jl and only afterwards src/trees/prufer.jl - so then the prufer_decode symbol simply does not exist.
But including src/trees/prufer.jl before would also not help, because that file relies on the definition of SimpleGraph.

I can't think of any pretty solution to that - but maybe we should simply move all the code into the SimpleGraphs submodule? Another solution would probably be, to declare an function without methods with

    function prufer_decode end

right before including src/SimpleGraphs/SimpleGraphs.jl - this is also not very nice though, because then we would keep the cyclic dependency.

@SimonCoste
Copy link
Contributor

I feel like including prufer_decode and other tree-related functions inside SimpleGraphs is really odd, since there are almost no other functions/algorithms implemented in SimpleGraphs.

What about modifying trees/prufer.jl so that it does not rely on SimpleGraphs ? It only uses the SimpleGraph{T} type in two places :

Maybe the simplest thing is to get rid of those in a consistent manner, by replacing them with Graph .

@simonschoelly simonschoelly mentioned this pull request Feb 9, 2023
@gdalle gdalle merged commit 4cef890 into JuliaGraphs:master Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants