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

New package: ImprovedHagerZhangLinesearch v0.1.0 #98950

Conversation

JuliaRegistrator
Copy link
Contributor

UUID: 36e1a5ba-44f2-43dc-a7bf-508830393e4a
Repo: https://github.com/mateuszbaran/ImprovedHagerZhangLinesearch.jl.git
Tree: c3d8ac52ba41653d4a13820cf1d112190dd7f5d5

Registrator tree SHA: 17aec322677d9b81cdd6b9b9236b09a3f1374c6a
JuliaRegistrator referenced this pull request in mateuszbaran/ImprovedHagerZhangLinesearch.jl Jan 16, 2024
Copy link
Contributor

Your new package pull request met all of the guidelines for auto-merging and is scheduled to be merged when the mandatory waiting period (3 days) has elapsed.

Since you are registering a new package, please make sure that you have read the package naming guidelines: https://julialang.github.io/Pkg.jl/dev/creating-packages/#Package-naming-guidelines-1


If you want to prevent this pull request from being auto-merged, simply leave a comment. If you want to post a comment without blocking auto-merging, you must include the text [noblock] in your comment. You can edit blocking comments, adding [noblock] to them in order to unblock auto-merging.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] Usually, you would open a pull request to LineSearches.jl with the fix, not register a new package

@mateuszbaran
Copy link
Contributor

I've opened an issue two weeks ago, with no response: JuliaNLSolvers/LineSearches.jl#173 , which to me indicates that no one would review such PR. I would be happy to make such PR if a maintainer expresses interest in reviewing it. In the meantime, a separate package makes sense to me, even more so because this package has some additional changes on top of that fix which wouldn't fit the original package.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] No. Registering a fork for an existing package would be an absolute last resort. Please make a pull request.

@mateuszbaran
Copy link
Contributor

Why would a maintainer consider my PR if they don't respond to an issue?

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] Why wouldn't they? What response exactly were you expecting on the issue? It is very routine for issues not to receive comments. They primarily exist as a TODO list of pull requests. Whoever is affected by the issue and motivated enough to provide a solution should open a PR. In this case, you.

If your PR doesn't receive any response, that's a different matter, but even then, you haven't nearly exhausted all your options. First, ping the maintainers on the issue. Remember that people might be busy/on holiday/travelling, so give it a couple of weeks. If the package turns out to be unmaintained, go on Slack and reach out to other members or owners of the JuliaNLSolvers organization and see if someone can merge your PR. Or, if the package is truly unmaintained, maybe they can add you as a maintainer, and you can merge and release your own PR.

Only after all of those things have failed would it be okay to register it as a new package. In the meantime, you can dev-install your fix from the PR branch.

@mateuszbaran
Copy link
Contributor

JuliaNLSolvers is know to be barely maintained. PRs are either not reviewed at all or it takes months to get a response -- I've submitted some myself in the past. I've been in contact with Patrick (the main maintainer) before (about different issues) and posted about my issue on the relevant Slack channel. So I'm not sure why you think it would be different for me now.

If your PR doesn't receive any response, that's a different matter, but even then, you haven't nearly exhausted all your options. First, ping the maintainers on the issue. Remember that people might be busy/on holiday/travelling, so give it a couple of weeks. If the package turns out to be unmaintained, go on Slack and reach out to other members or owners of the JuliaNLSolvers organization and see if someone can merge your PR. Or, if the package is truly unmaintained, maybe they can add you as a maintainer, and you can merge and release your own PR.

I'm not new to this community, I maintain a few packages myself. If ImprovedHagerZhangLinesearch is against the rules of general registry, just don't merge it, I don't care enough about it. Feel free to close this PR. By registering this package I just wanted to make life easier for people who would like to get the best performance from Manopt.jl. Anyway, they can dev the unregistered package too.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] It's not that it's "against the rules", although I'm not sure if the package name is the ideal choice. But if JuliaNLSolvers has a lack of maintainers, it would seem like the best way forward is to add more maintainers. Have you talked to the org owners about giving you maintainer access to LineSearches?

@pkofod Can you comment on the maintenance status of LineSearches?

Maybe @timholy and @KristofferC, as high-level members who have contributed to the repo?

@kellertuer You seem to indirectly have been involved in discussions about the underlying issue. Any insights?

@kellertuer
Copy link
Contributor

[noblock]
I am slightly involved in this, yes.
One reason for a separate package is, that in style, form, but especially documentation I would prefer to not have this in Manopt.jl. It is still great as a weak dependency (the same way LIneSearches.jl is).
The other reason is, as far as I understand, that this is even slightly more general than the LineSearches.jl ones concerning allocations, but on that I am not an expert but @mateuszbaran is ;)

So that is the two reasons I see. Sure this could in principle be included in LineSearches.jl – but I am not an expert in that package either, so I leave that evaluation to other people.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] Is there any chance we can come up with a better package name? I'm not going to stand in its way if others think it's okay, but "Improved" seems very unspecific for a package.

Does this package only include the HZ-method from LineSearches? If so, and if it's just a general continuation of the previous implementation in LineSearches, I see no reason why this couldn't be named HagerZhangLinesearch.jl. Even as a separate package, I also still think this is better as part of the JuliaNLSolvers org, with @mateuszbaran as the official maintainer of this new package. It's just better to have packages in org accounts with multiple owners, rather than a personal account, exactly to avoid these kinds of "unmaintained" issues.

@kellertuer
Copy link
Contributor

[nobock]
I can not tell more than I was told – I am quite happy that Mateusz works on improving performance for a package I do care about, though; your naming suggestions would hence also be fine with me.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

[noblock] If the org owners are unresponsive, this can also be moved from a personal account to an org at some later point (although that will require a manual PR to the registry). So again, I'm not going to stand in the way of this registration based on that.

@mateuszbaran
Copy link
Contributor

Have you talked to the org owners about giving you maintainer access to LineSearches?

No, I have not, but I don't think I'm the right person to maintain LineSearch. I feel like I'm already not doing a good enough job maintaining the packages I already do any my knowledge of line search algorithms is quite rudimentary.

The other reason is, as far as I understand, that this is even slightly more general than the LineSearches.jl ones concerning allocations, but on that I am not an expert but @mateuszbaran is ;)

Yes, on top of the fix I've changed the interface from LineSearches.jl style to Manopt.jl style because it allowed me to do some additional optimizations. Maybe they could also be achieved with LineSearches.jl interface but since I decided to fork, this was not a concern to me.

Is there any chance we can come up with a better package name? I'm not going to stand in its way if others think it's okay, but "Improved" seems very unspecific for a package.

Does this package only include the HZ-method from LineSearches? If so, and if it's just a general continuation of the previous implementation in LineSearches, I see no reason why this couldn't be named HagerZhangLinesearch.jl. Even as a separate package, I also still think this is better as part of the JuliaNLSolvers org, with @mateuszbaran as the official maintainer of this new package. It's just better to have packages in org accounts with multiple owners, rather than a personal account, exactly to avoid these kinds of "unmaintained" issues.

I'm open to using a different package name. Yes, it's just Hager-Zhang, though maybe I could also try the improved backtracking from JuliaNLSolvers/LineSearches.jl#172 . So I'm not even sure if restricting to HZ was the correct move. I started my work in a PR to Manopt but as Ronny wrote, we decided that it's better to put it in a separate package.

Let's keep this registration blocked for now and get some more input first.

@goerz
Copy link
Member

goerz commented Jan 16, 2024

That sounds good! Sorry that I started out a bit dismissive on this submission. At this point, at least with a revised name, I have absolutely no objections to this being registered as an independent package. [noblock]

@mateuszbaran
Copy link
Contributor

No problem, I'm also sometimes disappointed by people registering packages similar to existing ones without considering cooperation (though I don't comment on such submissions). I should've been more clear about my reasoning for registering this package.

@kellertuer
Copy link
Contributor

[noblock] Oh, no, thanks for all the questions and critically asking, I feel such discussions are super valuable, here it improved the name, and we could discuss why the rest is reasonable :)

@pkofod
Copy link
Contributor

pkofod commented Jan 16, 2024

I've opened an issue two weeks ago, with no response: JuliaNLSolvers/LineSearches.jl#173 , which to me indicates that no one would review such PR. I would be happy to make such PR if a maintainer expresses interest in reviewing it. In the meantime, a separate package makes sense to me, even more so because this package has some additional changes on top of that fix which wouldn't fit the original package.

I think that's a little harsh. Not everyone with an open source project has time daily to respond to issues. Two weeks may seem like a long time to some people. To others, the start of the year is a busy time at work :) I can read above you have also had other bad experiences to which I can only apologize, but many other people who find my response time to be too slow simply ping me on slack. I have written my slack tag in the Optim.jl readme a long time ago, but maybe I should do the same in the other repositories as well.

@kellertuer
Copy link
Contributor

kellertuer commented Jan 16, 2024

To be honest, having that fix in Linesearches.jl would also be my preferred way, but I currently do not have the capacity to help, that is why I only loosely followed with here.

edit: and for example I was not aware of the slack-ping-remark.

@mateuszbaran
Copy link
Contributor

I think that's a little harsh. Not everyone with an open source project has time daily to respond to issues. Two weeks may seem like a long time to some people.

I really appreciate your work on JuliaNLSolvers and I know that people are busy with other work (I'm falling behind in maintenance myself). You have no obligation to work as a maintainer. I just wanted to finally finish the Manopt improvements that I started working on a long time ago and that LineSearches issue was holding me back.

I think it's perfectly fine that people in a widely-used library need time to properly consider changes but if it takes months, it should be acceptable that people are looking for ways around it. For example the SciML ecosystem either already has or will soon have its own copy of most of the basic numerical ecosystem (including nonlinear solvers and optimization algorithms).

@timholy
Copy link
Member

timholy commented Jan 22, 2024

I'm not a fan of this either. If it's hard to maintain the packages we have, it will be even harder to maintain n times as many that are near duplicates of one another. In other words, fixing a problem this way today causes much bigger problems tomorrow.

Can we close this and get the fix in LineSolvers?

@mateuszbaran
Copy link
Contributor

We can close this PR but upstreaming all my improvements to LineSearches is quite unrealistic, so the duplication will happen in some form anyway.

@timholy
Copy link
Member

timholy commented Jan 22, 2024

If it's a review/merge bottleneck (and that's a common problem), then perhaps the solution is to give more people commit rights.

@mateuszbaran
Copy link
Contributor

Review bottleneck is one issue, another one is that at least one change would be breaking, and breaking changes in established packages are usually not well received unless it's a big improvement. Here it's a relatively minor optimization. I can easily do it in a fork but in LineSearches it would likely cause more annoyance than appreciation.

@timholy
Copy link
Member

timholy commented Jan 22, 2024

Is there a PR to LineSearches we can look at? Or a brief description?

@mateuszbaran
Copy link
Contributor

No, there is no PR. The breaking part is adding some caches to HagerZhang struct and making its construction dependent on the manifold we optimize on. I'm still working on these improvements.

@mateuszbaran
Copy link
Contributor

Funnily enough, Manopt.jl with this improved HZ linesearch is now faster than Optim.jl in Euclidean optimization (tested on Rosenbrock function, optimized using L-BFGS).

@timholy
Copy link
Member

timholy commented Jan 22, 2024

Sounds like good stuff. Without seeing the changes, obviously I can't say, but often something like adding caches can be done in a non-breaking manner. Anyway, I'd like to propose we close this and get it done in LineSearches.jl.

@pkofod
Copy link
Contributor

pkofod commented Jan 23, 2024

Sounds like good stuff. Without seeing the changes, obviously I can't say, but often something like adding caches can be done in a non-breaking manner. Anyway, I'd like to propose we close this and get it done in LineSearches.jl.

I agree and I believe my answers here and on slack reflect that Optim and other packages should not be seen as stale or deprecated. I have changed my notification settings on GitHub and added automation in my inbox such that I'll get to issues and PRs faster. Let me know if you would want to submit your changes to LineSearches.jl anyway.

Wrt the euclidean benchmarks, well. If it's across many problems I'm of course curious to have a look, but I'm wondering if it's overhead or algorithmic reasons. Optim.jl has quite some overhead which is part of why I wrote NLSolvers.jl. If it's because of the bug fix here, surely it would be nice if Optim.jl users could benefit as well :)

@timholy
Copy link
Member

timholy commented Jan 23, 2024

I interpret from the various thumbs-up that we have consensus. I'll close this.

@timholy timholy closed this Jan 23, 2024
@timholy timholy deleted the registrator-improvedhagerzhanglinesearch-36e1a5ba-v0.1.0-c0d5444d39 branch January 23, 2024 16:02
@mateuszbaran
Copy link
Contributor

I agree and I believe my answers here and on slack reflect that Optim and other packages should not be seen as stale or deprecated. I have changed my notification settings on GitHub and added automation in my inbox such that I'll get to issues and PRs faster. Let me know if you would want to submit your changes to LineSearches.jl anyway.

Right now I'm not sure what to submit, for now I'd just prefer to keep working in a separate repository. I can experiment with improvements more easily this way and later we can discuss what to backport to LineSearches.jl.

Wrt the euclidean benchmarks, well. If it's across many problems I'm of course curious to have a look, but I'm wondering if it's overhead or algorithmic reasons. Optim.jl has quite some overhead which is part of why I wrote NLSolvers.jl. If it's because of the bug fix here, surely it would be nice if Optim.jl users could benefit as well :)

L-BFGS in Manopt doesn't have any algorithmic improvements relative to Optim.jl for euclidean optimization. I've just lowered some overhead. It is significant if a function can be evaluated as quickly as Rosenbrock -- right now it's about 10% faster but with some improvements it could a bit better.

I have a general idea of investigating applications of Riemannian optimization to various problems. We (me and Ronny) recently managed to remove Manopt.jl's overhead compared to Optim.jl. I've started a hyperparameter tuning example for Manopt/Optim and I'm currently looking for some students to test it on some problems from multivariate data analysis. We will see how it works out. Removing small overhead from line search isn't particularly significant for this goal and my time is divided between multiple different projects so I'm not sure how far I will go in this direction. Picking initial and max alpha could be significant though.

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

Successfully merging this pull request may close these issues.

6 participants