JuliaGraphs / Graphs.jl

An optimized graphs package for the Julia programming language
http://juliagraphs.org/Graphs.jl/
Other
459 stars 91 forks source link

using symbols instead of structures when calling methods from ArnoldiMethod.jl #355

Closed gkantsidis closed 6 months ago

gkantsidis commented 6 months ago

According to https://github.com/JuliaLinearAlgebra/ArnoldiMethod.jl/commit/56221508b32e2be8cc041733f32f1fd669ba1490 the authors of ArnoldiMethods.jl would prefer users to use the symbols :LM, :SR, :LR, :SI, :LI instead of LM(), SR(), LR(), SI(), and LI() respectively. Accordingly, I have made those adjustments and removed the unecessary now imports of LM, SR, LR, SI and LI.

gdalle commented 6 months ago

Just because users can pass symbols doesn't mean they should. In particular, passing the two-letter structs (which are no longer exported but I took care of that in #136) allows type-stability, whereas the symbols probably introduce an additional runtime dispatch due to the conversion: https://github.com/JuliaLinearAlgebra/ArnoldiMethod.jl/blob/92f50945717cb3daa1ed237897deaa40d77f9535/src/run.jl#L181-L185

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 97.29%. Comparing base (de9cac7) to head (db65ba6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #355 +/- ## ======================================= Coverage 97.29% 97.29% ======================================= Files 117 117 Lines 6814 6814 ======================================= Hits 6630 6630 Misses 184 184 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gkantsidis commented 6 months ago

Although I also prefer the structs, I interpreted their commit comment as an attempt to hide the LR()/LM()/etc from the public interface. Now that I read their comment again, I may have over-interpreted it.

gdalle commented 6 months ago

It's ambiguous I agree :) Closing for now, let's keep the structs and if ArnoldiMethod changes again in a clearer way we'll reconsider!