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.

Sign up for GitHub

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

Timer interface improvement #3328

Merged
merged 10 commits into from
Sep 11, 2024
Merged

Timer interface improvement #3328

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
845518f
Adapt Timer
schnellerhase Jul 31, 2024
c9be2dd
Merge branch 'main' into optional
schnellerhase Sep 10, 2024
3c7c8c0
Remove space
schnellerhase Sep 10, 2024
4a127cf
Switch to member variable being optional
schnellerhase Sep 10, 2024
845a8a3
clang-format
schnellerhase Sep 10, 2024
0226a07
Fix python export
schnellerhase Sep 10, 2024
098ef4e
Remove move
schnellerhase Sep 10, 2024
c9eb49a
Remove none from python export
schnellerhase Sep 10, 2024
6e4eee3
Re add nb::none
schnellerhase Sep 10, 2024
888457f
Update python/dolfinx/wrappers/common.cpp
garth-wells Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 5 additions & 12 deletions cpp/dolfinx/common/Timer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,21 +7,14 @@
#include "Timer.h"
#include "TimeLogManager.h"
#include "TimeLogger.h"
#include <optional>
#include <stdexcept>

using namespace dolfinx;
using namespace dolfinx::common;

//-----------------------------------------------------------------------------
Timer::Timer() : Timer::Timer("")
{
// Do nothing
}
//-----------------------------------------------------------------------------
Timer::Timer(const std::string& task) : _task(task)
{
// Do nothing
}
Timer::Timer(std::optional<std::string> task) : _task(std::move(task)) {}
schnellerhase marked this conversation as resolved.
Show resolved Hide resolved
//-----------------------------------------------------------------------------
Timer::~Timer()
{
Expand All @@ -33,7 +26,7 @@ void Timer::start() { _timer.start(); }
//-----------------------------------------------------------------------------
void Timer::resume()
{
if (!_task.empty())
if (_task.has_value())
{
throw std::runtime_error(
"Resuming is not well-defined for logging timer. Only "
Expand All @@ -46,8 +39,8 @@ double Timer::stop()
{
_timer.stop();
const auto [wall, user, system] = this->elapsed();
if (!_task.empty())
TimeLogManager::logger().register_timing(_task, wall, user, system);
if (_task.has_value())
TimeLogManager::logger().register_timing(_task.value(), wall, user, system);
return wall;
}
//-----------------------------------------------------------------------------
Expand Down
13 changes: 7 additions & 6 deletions cpp/dolfinx/common/Timer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include <array>
#include <boost/timer/timer.hpp>
#include <optional>
#include <string>

namespace dolfinx::common
Expand All @@ -30,11 +31,11 @@ namespace dolfinx::common
class Timer
{
public:
/// Create timer without logging
Timer();

/// Create timer with logging
Timer(const std::string& task);
/// Create timer
///
/// If a task name is provided this enables logging to logger, otherwise (i.e.
/// no task provided) nothing gets logged.
Timer(std::optional<std::string> task = std::nullopt);

/// Destructor
~Timer();
Expand All @@ -54,7 +55,7 @@ class Timer

private:
// Name of task
std::string _task;
std::optional<std::string> _task;

// Implementation of timer
boost::timer::cpu_timer _timer;
Expand Down
5 changes: 1 addition & 4 deletions python/dolfinx/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,7 @@ class Timer:
_cpp_object: _cpp.common.Timer

def __init__(self, name: typing.Optional[str] = None):
if name is None:
self._cpp_object = _cpp.common.Timer()
else:
self._cpp_object = _cpp.common.Timer(name)
self._cpp_object = _cpp.common.Timer(name)

def __enter__(self):
self._cpp_object.start()
Expand Down
4 changes: 2 additions & 2 deletions python/dolfinx/wrappers/common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <nanobind/nanobind.h>
#include <nanobind/ndarray.h>
#include <nanobind/stl/array.h>
#include <nanobind/stl/optional.h>
#include <nanobind/stl/pair.h>
#include <nanobind/stl/string.h>
#include <nanobind/stl/tuple.h>
Expand Down Expand Up @@ -152,8 +153,7 @@ void common(nb::module_& m)
nb::arg("global"));
// dolfinx::common::Timer
nb::class_<dolfinx::common::Timer>(m, "Timer", "Timer class")
.def(nb::init<>())
.def(nb::init<std::string>(), nb::arg("task"))
.def(nb::init<std::string>(), nb::arg("task") = nb::none())
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove the default arg in the wrapper layer. Since we wrap the interface in the Python layer, best to handle the default arg there. Defaults in too many places can lead to bugs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does not work, faced the same problem within #3322 - I'm not sure what causes this or how it may be fixed, but somehow only when the nb::none() is provided it properly picks up on the python None to cpp std::nullopt conversion.

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at https://nanobind.readthedocs.io/en/latest/functions.html#none-arguments.

.def("start", &dolfinx::common::Timer::start, "Start timer")
.def("stop", &dolfinx::common::Timer::stop, "Stop timer")
.def("resume", &dolfinx::common::Timer::resume)
Expand Down
Loading