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

export profile data #142

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

d-monnet
Copy link
Contributor

No description provided.

@d-monnet d-monnet requested a review from tmigot October 12, 2023 14:01
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
src/profiles.jl 100.00% <100.00%> (ø)

📢 Thoughts on this report? Let us know!.

Copy link
Member

@tmigot tmigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @d-monnet ! Thank you for the PR.

Connected to #140 (comment) do you think we would also need such function in BenchmarkProfiles.jl ?

Could you also think about some unit tests for this new functionality?

@d-monnet
Copy link
Contributor Author

Hi @d-monnet ! Thank you for the PR.

Connected to #140 (comment) do you think we would also need such function in BenchmarkProfiles.jl ?

I'm not sure a .csv export function would be needed in BenchmarkProfiles.jl which is to me too low level for the generic user, but it would require almost zero work since it would be pretty much a copy of what's in get_profile_solvers_data and export_profile_solvers_data. Let me know if you want me to do that.

Could you also think about some unit tests for this new functionality?

Sure, I'll add the tests. Is it possible to create a file and load it through githut actions though ?

@tmigot
Copy link
Member

tmigot commented Oct 13, 2023

Ok, so let's do it in BenchmarkProfiles.jl first then, if that's ok for you.

I see it is possible for the file. I could imagine the following process: call the function, test that the file exists, and remove the file.
Let me know if that doesn't work.

@d-monnet
Copy link
Contributor Author

Here is the PR for csv export in BenchmarkProfiles.jl: JuliaSmoothOptimizers/BenchmarkProfiles.jl#95

@tmigot
Copy link
Member

tmigot commented Oct 18, 2023

Hi @d-monnet , let me know when I can review this. Do you need a new release of BenchmarkProfiles.jl for this?

@d-monnet
Copy link
Contributor Author

d-monnet commented Oct 18, 2023

Hi @d-monnet , let me know when I can review this. Do you need a new release of BenchmarkProfiles.jl for this?

Sure, still working on it but it should be done (tests included ;) ) by the end of the week.
A new release of BenchmarkProfiles.jl would avoid code duplication in this package, if it's ok. However I think there is a compatibility issue for Tables versions between BenchmarkProfile.jl and SolverBenchmark.jl...

@tmigot
Copy link
Member

tmigot commented Oct 19, 2023

There is a BenchmarkProfile.jl 0.4.4 with the new feature.
What is the compatibility issue?

@d-monnet
Copy link
Contributor Author

There is a BenchmarkProfile.jl 0.4.4 with the new feature. What is the compatibility issue?

The new release of BenchmarkProfiles.jl requires Tables.jl 1.1 and PrettyTables.jl in SolverBenchmark.jl requires an older version.

@d-monnet
Copy link
Contributor Author

Hi @tmigot, the PR is ready for review.

Copy link
Member

@tmigot tmigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @d-monnet ! thank you for the PR (and sorry for the late review). It looks good overall, just some minor things about the documentation. We should also try to unify the punctuation on bullet list.
I will try to investigate why the tests are not passing for the documentation and julia 1.6.

src/profiles.jl Show resolved Hide resolved
src/profiles.jl Outdated Show resolved Hide resolved
src/profiles.jl Outdated Show resolved Hide resolved
src/profiles.jl Outdated Show resolved Hide resolved
src/profiles.jl Outdated Show resolved Hide resolved
@tmigot tmigot mentioned this pull request Oct 23, 2023
@tmigot
Copy link
Member

tmigot commented Oct 23, 2023

I think the failed doc is unrelated to this.
The CI Julia 1.6 failure, however is. It is probably because a file is not correctly removed ? The shortcut would be to modify https://github.com/JuliaSmoothOptimizers/SolverBenchmark.jl/blob/main/test/runtests.jl and tests pkgbmark.jl before the rest.

src/profiles.jl Outdated Show resolved Hide resolved
src/profiles.jl Outdated Show resolved Hide resolved
@tmigot
Copy link
Member

tmigot commented Oct 23, 2023

Ok, so we will have to investigate further to see what is the problem with Julia 1.6... Otherwise it is good for me. Thank you!

@d-monnet
Copy link
Contributor Author

Based on that branch I have developed a function to export the profile figures as tikz, which I prefer to include in my papers rather than the .png exported by performance_profile().
Let me know if/how you want me to add this contribution. Not sure how to do it properly until this PR is merged...

@tmigot
Copy link
Member

tmigot commented Oct 24, 2023

Based on that branch I have developed a function to export the profile figures as tikz, which I prefer to include in my papers rather than the .png exported by performance_profile(). Let me know if/how you want me to add this contribution. Not sure how to do it properly until this PR is merged...

Would that involve additional packages?

@d-monnet
Copy link
Contributor Author

Based on that branch I have developed a function to export the profile figures as tikz, which I prefer to include in my papers rather than the .png exported by performance_profile(). Let me know if/how you want me to add this contribution. Not sure how to do it properly until this PR is merged...

Would that involve additional packages?

No, the whole figure source code is "hand made" from profiles plots points.

@tmigot
Copy link
Member

tmigot commented Oct 24, 2023

Ok, so why not. Maybe we do the same and add it to BenchmarkProfiles first ?

@d-monnet
Copy link
Contributor Author

Ok, so why not. Maybe we do the same and add it to BenchmarkProfiles first ?

Here is the PR: JuliaSmoothOptimizers/BenchmarkProfiles.jl#104

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

Successfully merging this pull request may close these issues.

2 participants