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

Consolidate Stats Handling #437

Merged
merged 4 commits into from
May 25, 2024
Merged

Consolidate Stats Handling #437

merged 4 commits into from
May 25, 2024

Conversation

avik-pal
Copy link
Member

@avik-pal avik-pal commented May 25, 2024

Fixes #410

Oops lost my local changes it seems...

Merge after #435

@avik-pal avik-pal changed the title [WIP] Consolidate Stats Handling Consolidate Stats Handling May 25, 2024
src/core/approximate_jacobian.jl Outdated Show resolved Hide resolved
src/core/approximate_jacobian.jl Outdated Show resolved Hide resolved
src/core/generalized_first_order.jl Outdated Show resolved Hide resolved
src/core/generalized_first_order.jl Outdated Show resolved Hide resolved
src/core/generic.jl Outdated Show resolved Hide resolved
src/globalization/line_search.jl Outdated Show resolved Hide resolved
src/globalization/trust_region.jl Outdated Show resolved Hide resolved
src/internal/jacobian.jl Outdated Show resolved Hide resolved
src/internal/jacobian.jl Outdated Show resolved Hide resolved
src/utils.jl Outdated Show resolved Hide resolved
@avik-pal
Copy link
Member Author

Now cache.stats gives the total counts correctly. For polyalgorithms this means we get the accumulated stats from all the solve calls.

@avik-pal
Copy link
Member Author

cc @oscardssmith because you needed this in OrdinaryDiffEq.

@avik-pal avik-pal force-pushed the ap/consolidate_stats branch from 2ff5aa6 to e67f4f2 Compare May 25, 2024 02:20
@avik-pal avik-pal force-pushed the ap/consolidate_stats branch from e67f4f2 to e0154f6 Compare May 25, 2024 02:21
src/internal/helpers.jl Outdated Show resolved Hide resolved
@avik-pal avik-pal force-pushed the ap/consolidate_stats branch from e0154f6 to f825754 Compare May 25, 2024 02:22
Copy link

codecov bot commented May 25, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 86.42%. Comparing base (86bb307) to head (50aa5c4).
Report is 2 commits behind head on master.

Files Patch % Lines
src/default.jl 75.00% 1 Missing ⚠️
src/descent/steepest.jl 0.00% 1 Missing ⚠️
src/internal/jacobian.jl 87.50% 1 Missing ⚠️
src/internal/operators.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #437      +/-   ##
==========================================
- Coverage   86.46%   86.42%   -0.05%     
==========================================
  Files          47       47              
  Lines        2904     2865      -39     
==========================================
- Hits         2511     2476      -35     
+ Misses        393      389       -4     

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

@avik-pal avik-pal requested a review from ChrisRackauckas May 25, 2024 02:59
@avik-pal avik-pal force-pushed the ap/consolidate_stats branch from f825754 to 066912c Compare May 25, 2024 15:46
@avik-pal avik-pal mentioned this pull request May 25, 2024
1 task
@avik-pal avik-pal merged commit 630aad4 into master May 25, 2024
25 of 26 checks passed
@avik-pal avik-pal deleted the ap/consolidate_stats branch May 25, 2024 23:30
@oscardssmith
Copy link
Contributor

This change appears to have broken SciML/OrdinaryDiffEq.jl#2167. Do you want to make the corresponding OrdinaryDiffEq change or should I?

@avik-pal
Copy link
Member Author

Can you do it? I am not sure what broke, this was a purely internal API change

@oscardssmith
Copy link
Contributor

will do. The problem was that OrdinaryDiffEq with NonlinearSolveAlg runs the step!(cache) loops of a nonlinear solver itself and was using nf to determine how many calls the step! had made to update the integrator's stats.

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.

Consolidate stats handling
3 participants