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

Use metaprogramming to define the kwargs of all Krylov methods #735

Merged
merged 3 commits into from
May 10, 2023

Conversation

amontoison
Copy link
Member

@amontoison amontoison commented May 9, 2023

Thanks @mosullivan93 for your help with metaprogramming!
@dpo @tmigot Julia will not recompile the non in-place methods anymore if we use different keyword arguments.

It will be also easier to maintain.

ps: I found some mistakes in the docstrings of CRLS, CGLS, CGNE and CRMR.

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Patch coverage: 99.71% and project coverage change: -0.01 ⚠️

Comparison is base (cec1dd8) 98.28% compared to head (a6b2b04) 98.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
- Coverage   98.28%   98.27%   -0.01%     
==========================================
  Files          38       38              
  Lines        6861     6898      +37     
==========================================
+ Hits         6743     6779      +36     
- Misses        118      119       +1     
Impacted Files Coverage Δ
src/krylov_utils.jl 91.32% <75.00%> (-0.35%) ⬇️
src/bicgstab.jl 100.00% <100.00%> (ø)
src/bilq.jl 100.00% <100.00%> (ø)
src/bilqr.jl 100.00% <100.00%> (ø)
src/cg.jl 100.00% <100.00%> (ø)
src/cg_lanczos.jl 100.00% <100.00%> (ø)
src/cg_lanczos_shift.jl 100.00% <100.00%> (ø)
src/cgls.jl 100.00% <100.00%> (ø)
src/cgne.jl 100.00% <100.00%> (ø)
src/cgs.jl 100.00% <100.00%> (ø)
... and 24 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

Package name latest stable
CaNNOLeS.jl
DCISolver.jl
FletcherPenaltySolver.jl
JSOSolvers.jl
LLSModels.jl
Percival.jl
RipQP.jl

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.

Thanks @amontoison !
It looks good to me. I guess this will increase the pre-compilation of the package though, but that's alright for now.

@amontoison
Copy link
Member Author

amontoison commented May 10, 2023

No, the functions were already precompiled.
The only difference now is that the non-inplace methods know which in-place version they will call, which means that Julia doesn't need to recompile them at runtime to fully infer the code.
Julia was unable to predict the kwargs....

The in-place methods (without warm-start) were already correctly precompiled.

@amontoison amontoison merged commit de52dfe into JuliaSmoothOptimizers:main May 10, 2023
@amontoison amontoison deleted the optargs_kwargs branch May 10, 2023 14:18
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.

2 participants