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

Algorithm for finding the longest path of a DAG #209

Merged
merged 12 commits into from
Mar 5, 2024

Conversation

matheusdiogenesandrade
Copy link
Contributor

DAG longest path algorithm.

@matheusdiogenesandrade
Copy link
Contributor Author

Could someone help me with this test case?

Code formatting (JuliaFormatter.jl): Test Failed at /home/runner/work/Graphs.jl/Graphs.jl/test/runtests.jl:24
  Expression: format(Graphs; verbose = false, overwrite = false, ignore = ["vf2.jl"])

Even locally I am not able to pass it.

@aurorarossi
Copy link
Member

I had the same problem (I solved it in a PR that is still in progress).
It should work by removing the parentheses in the runtest.jl file here: ignore = ["vf2.jl"] --> ignore = "vf2.jl".

@simonschoelly
Copy link
Contributor

I had the same problem (I solved it in a PR that is still in progress). It should work by removing the parentheses in the runtest.jl file here: ignore = ["vf2.jl"] --> ignore = "vf2.jl".

If you have time, maybe you can create a separate PR just for that change, so that we could merge it quickly?

@matheusdiogenesandrade
Copy link
Contributor Author

@aurorarossi Thanks, just made this, let's see now. @simonschoelly, I pushed the changes, what do you mean by making another PR? I am sorry, but this is my first open-source contribution, thus I am not used to some usual tasks.

@aurorarossi
Copy link
Member

aurorarossi commented Jan 4, 2023

@aurorarossi Thanks, just made this, let's see now. @simonschoelly, I pushed the changes, what do you mean by making another PR? I am sorry, but this is my first open-source contribution, thus I am not used to some usual tasks.

I think that @simonschoelly was referring to me. Do not worry, I will take care of it :).

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.29%. Comparing base (da6f801) to head (989b819).
Report is 15 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   97.26%   97.29%   +0.03%     
==========================================
  Files         115      117       +2     
  Lines        6795     6814      +19     
==========================================
+ Hits         6609     6630      +21     
+ Misses        186      184       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matheusdiogenesandrade
Copy link
Contributor Author

@aurorarossi thanks. Folks, concerning this last checking (https://github.com/JuliaGraphs/Graphs.jl/pull/209/checks?check_run_id=10444689306), can someone help me to understand why it is failing?

Thanks.

@aurorarossi
Copy link
Member

It means that some lines are not covered by a test.

@matheusdiogenesandrade
Copy link
Contributor Author

@aurorarossi Thank you! So I think that providing unitary tests (as I did https://github.com/JuliaGraphs/Graphs.jl/pull/209/files#diff-9c1223f541fc1697a6f988c8c086087d93a6467d5f7da2a2d9b2cd612c92184f) is not enough. What sort of additional tests should I provide?

Regards.

@matheusdiogenesandrade
Copy link
Contributor Author

matheusdiogenesandrade commented Jan 5, 2023

@aurorarossi never mind, I forgot to insert my test file in the runtests.jl.

@gdalle gdalle added the enhancement New feature or request label Jun 16, 2023
@matheusdiogenesandrade
Copy link
Contributor Author

Sorry, I am quite lost on how to solve these conflicts. Does anyone have any suggestion?

Thnx, and regards.

@gdalle
Copy link
Member

gdalle commented Dec 21, 2023

@matheusdiogenesandrade thank you for the contribution!

I brought it up to speed with the latest changes and I simplified the syntax a bit. The doctest is still unreliable (it might fail or succeed), but it would be interesting for you to try and figure out why on your own :) If you don't find the reason, I can help.

As a side note, you don't need to declare the type of every object you create, in most cases Julia will infer it for you!

@matheusdiogenesandrade
Copy link
Contributor Author

@matheusdiogenesandrade thank you for the contribution!

I brought it up to speed with the latest changes and I simplified the syntax a bit. The doctest is still unreliable (it might fail or succeed), but it would be interesting for you to try and figure out why on your own :) If you don't find the reason, I can help.

As a side note, you don't need to declare the type of every object you create, in most cases Julia will infer it for you!

Thanks. I think I solved the issue.

@gdalle
Copy link
Member

gdalle commented Dec 21, 2023

You did but you overwrote my previous changes to the longest paths source file in the process. Can you revert your latest commit and just change the necessary seed?

More tests would also be welcome with a different topological order

@matheusdiogenesandrade
Copy link
Contributor Author

You did but you overwrote my previous changes to the longest paths source file in the process. Can you revert your latest commit and just change the necessary seed?

More tests would also be welcome with a different topological order

Sorry, it was an accident, but now I think it is ok. Are the erros with the nightly julia version normal?

Regards.

@gdalle gdalle merged commit d1081b0 into JuliaGraphs:master Mar 5, 2024
12 checks passed
@matheusdiogenesandrade
Copy link
Contributor Author

Thank very much everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants