-
Notifications
You must be signed in to change notification settings - Fork 102
add non-allocating enumerate_paths! #428
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
Conversation
Bump |
the failures seem to be only in JuliaFormating tests. Do you mind running the formater? |
Ok, thats done. I have a few requests:
|
I am not really a core part of the team, so I can not address these requests, but I am in favor of the changes you are requesting. @gdalle , are you a good person to tag on this (see previous comment)? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #428 +/- ##
==========================================
+ Coverage 97.31% 97.41% +0.10%
==========================================
Files 117 120 +3
Lines 6956 6959 +3
==========================================
+ Hits 6769 6779 +10
+ Misses 187 180 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I am rerunning the timed-out jobs now. |
Sorry @Krastanov, as I announced on Slack and Discourse a while back, I have decided to retreat from JuliaGraphs maintenance tasks. I already have much on my plate with the JuliaDiff ecosystem, so it wouldn't be fair to pretend that I can take care of a second large project. |
@gdalle , @etiennedeg , @simonschoelly , I am happy to take on some small part of the review and support burden.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I'm also happy to help out a bit, Im committed to some packages building on the graphs ecosystem. |
Thanks! You were added to the list of maintainers! I have left a review approving of a merge, so the merge option should now be unlocked for you. Please feel free to squash and merge unless there are other changes you would like to discuss. |
Guys, format is deleting things... and having formats mixed with my PR code is making it look pretty awful. This stuff has now been 3x the work of writing the alg, which took 10 minutes |
This seems to be an issue with JuliaFormatter |
a1f8b6f
to
14bb353
Compare
Yeah, but it does add to my suggestion to not have these things enforced for PRs, but later with a bot where we can ignore it when its broken |
I am sorry for the bad experience -- it is difficult to balance defensive aggressive linting with approachability for new contributors with different workflows when a project is starved for volunteers. I acknowledge that this leads to negative feedback loop on the number of volunteers though. I will try to add a github action that does the reformatting automatically on each PR. Hopefully that would alleviate this particular issue. |
Thanks. I'm on extremely limited time and stealing time to finish this so small delays lead to weeks. For efficiency doing this inside our framework would have been much easier, but its totally unprincipled to do that. I'm just hoping we can keep people making the principled decision to contribute small improvements to Graphs.jl, and for that I believe the overheads need to be lower. |
Ok I restarted from main and this seems ok now, so good to merge from my perpective (also can we get a patch version registered with the merge? so ConScape.jl can use it!) |
Tests pass, formatting passes, @simonschoelly 's comments are addressed, and a quick cursory additional review from me does not uncover issues. Merging. Thank you for sticking with this! I will try to make future reviews less painful! |
This PR adds an in-place version of
enumerate_paths
-enumerate_paths!
, thatenumerate_paths
now calls itself after making the required allocations.This can greatly improve performance where you need to call it many times. I also removed allocations in
enumerate_paths
functions, such as allocations ofVector{Int}
whereUnitRange
works fine in its place.(This may need a specific test, although it will run in the current tests anyway - I can write that if its considered to be merged)