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

Count another location of KKT.solve!() call in the final stats (previously unreported). #147

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

pratyai
Copy link
Contributor

@pratyai pratyai commented Jun 7, 2024

Since the KKT.solve!() is typically one of the more expensive parts of a Newton step computation, we should account for the ~20% of calls and runtime that are currently unreported.

@mtanneau
Copy link
Member

mtanneau commented Jun 7, 2024

Thanks @pratyai ! Could you share a screenshot of what the final TimerOutputs displays to after this change please?
I'm trying to understand the interplay between the "Newton" and "KKT" tags in the @timeit macros, and how it will interact with the timing report of solve_newton_system! later (which is also tagged as "Newton" in @timeit)

@pratyai
Copy link
Contributor Author

pratyai commented Jun 7, 2024

Previously, at every step, one kkt.solve!() call was under-reported (e.g. in the picture it increases by exactly 12, after I add the @timeit call).

The "KKT" stats are nested inside the "Newton" stats, which is why I had to do the nesting with @timeit macro, so that it doesn't get reported separately (although perhaps it should be).

Before

Screenshot 2024-06-07 at 16 06 01

After

Screenshot 2024-06-07 at 16 04 02

@mtanneau mtanneau merged commit f3df04d into ds4dm:master Jun 9, 2024
10 checks passed
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