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

Introduce Timer context manager #995

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Introduce Timer context manager #995

merged 1 commit into from
Jan 23, 2024

Conversation

guyer
Copy link
Member

@guyer guyer commented Jan 19, 2024

Originally tried to make Timer() do logging, too, but I couldn't figure out how to get it to log in the correct scope.
Separating timing from logging substantially simplifies the logic at a modest cost in LoC at point of use.

Originally tried to make it do logging, too, but I couldn't figure
out how to get it to log in the correct scope.
Separating timing from logging substantially simplifies the logic
at a modest cost in LoC at point of use.
@guyer guyer requested review from wd15 and tkphd January 19, 2024 20:10
Copy link
Contributor

@tkphd tkphd left a comment

Choose a reason for hiding this comment

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

The code LGTM. The output looks like (with input blocks escaped):

$ export FIPY_LOG_CONFIG=fipy/tools/logging/serial_config.json
$ python3 simple.py
Solver suite is scipy
{'python': '3.11.5 (main, Sep 11 2023, 13:54:46) [GCC 11.2.0]', 'fipy': '3.4.4+10.g626e154ca', 'numpy': '1.26.3', 'pysparse': 'not installed', 'scipy': '1.11.4', 'matplotlib': '3.8.2', 'mpi4py': 'not installed', 'petsc4py': 'not installed', 'pyamgx': 'not installed', 'PyTrilinos': 'not installed', 'mayavi': 'not installed', 'gmsh': '4.12.0', 'solver': 'scipy'}
False
True
True
True
True
True
True
$ grep -E "[[:digit:]]+[[:space:]]ns" fipy.log
2024-01-19 16:35:15,377 - DEBUG - fipy.terms.binaryTerm._BinaryTerm - _prepareLinearSystem - END _prepareLinearSystem - 7706122 ns
2024-01-19 16:35:15,377 - DEBUG - fipy.solvers.scipy.linearGMRESSolver.LinearGMRESSolver - _solve_ - END precondition - 512 ns
2024-01-19 16:35:15,377 - DEBUG - fipy.solvers.scipy.linearGMRESSolver.LinearGMRESSolver - _solve_ - END solve - 64355 ns
2024-01-19 16:35:15,377 - DEBUG - fipy.terms.binaryTerm._BinaryTerm - solve - END solve - 7903306 ns
2024-01-19 16:35:15,385 - DEBUG - fipy.terms.binaryTerm._BinaryTerm - _prepareLinearSystem - END _prepareLinearSystem - 7594736 ns
2024-01-19 16:35:15,385 - DEBUG - fipy.solvers.scipy.linearLUSolver.LinearLUSolver - _solve_ - END precondition - 33168 ns
2024-01-19 16:35:15,386 - DEBUG - fipy.solvers.scipy.linearLUSolver.LinearLUSolver - _solve_ - END solve - 299729 ns
2024-01-19 16:35:15,386 - DEBUG - fipy.terms.binaryTerm._BinaryTerm - solve - END solve - 8239543 ns

@wd15
Copy link
Contributor

wd15 commented Jan 22, 2024

LGTM, I get similar to @tkphd.

I see that the Timer context is being repeated in many different places. Suggests that the context should be in a single place and then what's inside the context should be what is unique to each subclass.

@guyer
Copy link
Member Author

guyer commented Jan 23, 2024

I see that the Timer context is being repeated in many different places. Suggests that the context should be in a single place and then what's inside the context should be what is unique to each subclass.

I see your point. The thing is that what's inside the context is highly dependent on the broader context of ._solve_(). I guess that could be stored as a bunch of Solver member variables... I'll think about this in my broader solver refactorization.

@guyer guyer merged commit e290e35 into master Jan 23, 2024
24 checks passed
@tkphd tkphd deleted the add_timer branch January 23, 2024 16:29
@tkphd tkphd restored the add_timer branch January 23, 2024 16:29
@tkphd tkphd deleted the add_timer branch January 23, 2024 16:30
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.

None yet

3 participants