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

Implement simulation_clock type #881

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
219 changes: 219 additions & 0 deletions include/simulation_time/simulation_clock.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
#pragma once

#include <cassert>
#include <chrono>
#include <stdexcept>
#include <utility>

namespace ngen {

//! Simulation Clock
//!
//! Represents simulation ticks monotonically increasing from zero, and
//! provides conversion utilities between simulation ticks and
//! calendar time.
//!
//! Implements named requirements: Clock (https://en.cppreference.com/w/cpp/named_req/Clock)
Copy link
Contributor

Choose a reason for hiding this comment

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

Clock or TrivialClock? This disagrees with the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both, though the intention is to implement Clock -- it just happens to also implement TrivialClock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed after posting this question that TrivialClock is actually the more demanding of the two concepts. I think it's fine to just reference Clock in the code, to avoid confusion, though maybe mention it in the PR description.

//!
struct simulation_clock final
{
/* Clock named requirements */

using rep = std::int64_t; // TODO: double vs std::int64_t
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're likely to run into sub-second time precision, so int64_t is probably fine.

using period = std::ratio<1>;
using duration = std::chrono::duration<rep, period>;
using time_point = std::chrono::time_point<simulation_clock, duration>;
static constexpr bool is_steady = false;

//! Current Simulation Time
//! \returns The current point in time the simulation is at.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost philosophical, but in a sense, the description of the returned value is premised on the expected use by callers of now() and tick() and the epoch routines.

To be really strict, it's really more like "The accumulation of argument values from preceding calls to tick()"

//! \post Return value must have a non-negative representation value..
static time_point now() noexcept;

/* simulation_clock-specific types/interface */

// explicit simulation time type aliases //
using simulation_rep = rep;
using simulation_period = period;
using simulation_duration = duration;
using simulation_time_point = time_point;

// explicit calendar time type aliases //
using calendar_clock = std::chrono::system_clock;
using calendar_rep = calendar_clock::rep;
using calendar_period = calendar_clock::period;
using calendar_duration = calendar_clock::duration;
using calendar_time_point = calendar_clock::time_point;

// simulation-calendar conversion functions //

//! Convert a calendar time point to a simulation time point.
//! \param calendar_time Calendar time point.
//! \returns The corresponding simulation time point.
//! \pre simulation_clock::set_epoch must have been called prior to calling this.
static simulation_time_point from_calendar(const calendar_time_point& calendar_time);

//! Convert a simulation time point to a calendar time point.
//! \param sim_time Simulation time point.
//! \returns The corresponding calendar time point.
//! \pre simulation_clock::set_epoch must have been called prior to calling this.
static calendar_time_point to_calendar(const simulation_time_point& sim_time);

//! Convert a std::time_t value to a simulation time point.
//! \see simulation_clock::from_calendar
//! \param time Unix epoch-based time value
//! \returns The corresponding simulation time point.
//! \pre simulation_clock::set_epoch must have been called prior to calling this.
static simulation_time_point from_time_t(std::time_t t);

//! Convert a simulation time point to a std::time_t value.
//! \see simulation_clock::to_calendar
//! \param sim_time Simulation time point.
//! \returns The corresponding std::time_t value.
//! \pre simulation_clock::set_epoch must have been called prior to calling this.
static std::time_t to_time_t(const simulation_time_point& sim_time);

// simulation management functions //

//! Uninitialized epoch value, aka the epoch of typename simulation_clock::calendar_clock.
static constexpr calendar_time_point uninitialized_epoch() noexcept;

//! Get the simulation epoch.
//! \see simulation_clock::calendar_time_point
//! \see simulation_clock::set_epoch
//! \returns
//! Calendar time point representing the simulation start date/time.
//! If the epoch has not been set, then simulation_clock::uninitialized_epoch
//! is returned.
static calendar_time_point epoch() noexcept;

//! Set the simulation epoch.
//! \note This function must only be called once.
//! \param calendar_time Calendar time set as the epoch.
//! \throws std::logic_error Thrown when this function is called more than once.
static void set_epoch(calendar_time_point calendar_time);

//! Check if the simulation epoch has been set.
//! \returns true when the epoch is set, and false otherwise.
static bool has_epoch() noexcept;

//! Forward the simulation by N ticks.
//! \param steps Number of steps to tick.
//! \note Ticking does *not* require that simulation_clock::set_epoch has been called.
//! \see simulation_clock::period
//! \pre `steps` must be non-negative.
//! \throws std::domain_error Throws when the precondition is violated.
static void tick(rep steps = 1);
program-- marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this overload tick(rep) at all? When do we want calls that aren't in terms of a duration type?

If we do keep it, I think the parameter name steps is wrong. It's unit seconds, while the model time step is typically going to be something else. Is this maybe a hold-over from an earlier version you drafted with a time_step member variable that would have been set? Given the inherent static nature of Clock, I'd lean against bringing that back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm that's a good point, I don't think we need this. I added this prior to really thinking about how duration casting could better represent arbitrary ticking


//! Forward the simulation by some duration.
//! \param duration Duration to move simulation forward.
//! \note Ticking does *not* require that simulation_clock::set_epoch has been called.
//! \pre `duration` must be non-negative.
//! \throws std::domain_error Throws when the precondition is violated.
template<typename Rep, typename Period>
static void tick(const std::chrono::duration<Rep, Period>& duration);

private:
//! Simulation epoch, the calendar start time of the simulation.
//! \returns Mutable reference to the internal calendar epoch.
static calendar_time_point& internal_epoch();

//! Current simulation tick.
//! \returns Mutable reference to the current internal simulation tick.
static simulation_time_point& internal_now();
};

/* simulation_clock Implementation */

inline constexpr simulation_clock::calendar_time_point simulation_clock::uninitialized_epoch() noexcept
{
return {};
}

inline simulation_clock::calendar_time_point& simulation_clock::internal_epoch()
{
static calendar_time_point epoch = simulation_clock::uninitialized_epoch();
return epoch;
}

inline simulation_clock::time_point& simulation_clock::internal_now()
{
static simulation_time_point now{}; // Default initialize to zero value
return now;
}
Comment on lines +139 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this do the right thing with linking multiple objects that #include <simulation_clock.hpp>, or do we need to put this and internal_epoch (and presumably the rest of the implementations) in a separate .cpp file?

Is there some benefit to inlining all of this that's worth more than not having to think about the above question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch, yeah this might be incorrect. No actual reason for it being header-only, haha


inline simulation_clock::time_point simulation_clock::now() noexcept
{
return simulation_clock::internal_now();
}

inline simulation_clock::time_point simulation_clock::from_calendar(const calendar_time_point& calendar_time)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
inline simulation_clock::time_point simulation_clock::from_calendar(const calendar_time_point& calendar_time)
inline simulation_clock::simulation_time_point simulation_clock::from_calendar(const calendar_time_point& calendar_time)

{
assert(simulation_clock::has_epoch());

return simulation_time_point{
std::chrono::duration_cast<simulation_duration>(simulation_clock::internal_epoch() - calendar_time)
};
}

inline simulation_clock::calendar_time_point simulation_clock::to_calendar(const simulation_time_point& sim_time)
{
assert(simulation_clock::has_epoch());

simulation_duration sim_duration = sim_time.time_since_epoch();
return simulation_clock::internal_epoch() + std::chrono::duration_cast<calendar_duration>(sim_duration);
}

inline simulation_clock::time_point simulation_clock::from_time_t(std::time_t t)
{
assert(simulation_clock::has_epoch());

return simulation_clock::from_calendar(calendar_clock::from_time_t(t));
}

inline std::time_t simulation_clock::to_time_t(const simulation_time_point& sim_time)
{
assert(simulation_clock::has_epoch());

return calendar_clock::to_time_t(simulation_clock::to_calendar(sim_time));
}

inline simulation_clock::calendar_time_point simulation_clock::epoch() noexcept
{
return simulation_clock::internal_epoch();
}

inline void simulation_clock::set_epoch(calendar_time_point calendar_time)
{
if (simulation_clock::has_epoch()) {
throw std::logic_error{"Attempting to set simulation_clock epoch after already being set."};
}

simulation_clock::internal_epoch() = std::move(calendar_time);
}

inline bool simulation_clock::has_epoch() noexcept
{
return simulation_clock::internal_epoch() != simulation_clock::uninitialized_epoch();
}

inline void simulation_clock::tick(rep steps)
{
if (steps < 0) {
throw std::domain_error{"Given `steps` is negative when it must be non-negative."};
}

simulation_clock::internal_now() += simulation_clock::duration{steps};
}

template<typename Rep, typename Period>
inline void simulation_clock::tick(const std::chrono::duration<Rep, Period>& duration)
{
if (duration < std::chrono::duration<Rep, Period>::zero()) {
throw std::domain_error{"Given `duration` is negative when it must be non-negative."};
}

simulation_clock::internal_now() += std::chrono::duration_cast<simulation_clock::duration>(duration);
}

} // namespace ngen
1 change: 1 addition & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ ngen_add_test(
forcing/NetCDFPerFeatureDataProvider_Test.cpp
forcing/GridDataSelector_Test.cpp
core/mediator/UnitsHelper_Tests.cpp
simulation_time/simulation_clock_Test.cpp
simulation_time/Simulation_Time_Test.cpp
core/NetworkTests.cpp
utils/include/StreamOutputTest.cpp
Expand Down
47 changes: 47 additions & 0 deletions test/simulation_time/simulation_clock_Test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#include "gtest/gtest.h"

#include <simulation_time/simulation_clock.hpp>

using clock_type = ngen::simulation_clock;

// 1704067200 -> 01-01-2024 00:00:00 UTC
constexpr std::time_t epoch_unix = 1704067200L;
const auto epoch_calendar = clock_type::calendar_clock::from_time_t(epoch_unix);

TEST(SimulationClockTest, TestConfiguration)
{
ASSERT_FALSE(clock_type::has_epoch());
ASSERT_NO_THROW(clock_type::set_epoch(epoch_calendar));
ASSERT_TRUE(clock_type::has_epoch());
ASSERT_THROW(clock_type::set_epoch(epoch_calendar), std::logic_error);
EXPECT_EQ(clock_type::epoch(), epoch_calendar);
}

TEST(SimulationClockTest, TestConversion)
Copy link
Contributor

@PhilMiller PhilMiller Sep 11, 2024

Choose a reason for hiding this comment

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

It looks like this test assumes TestConfiguration has already run before it, setting the simulation_clock's epoch. With tests run in parallel, is that dependency still honored? I don't know how concurrent gtest can get, and whether our usage of it otherwise allows for that concurrency.

In general, it's not great to have inter-test dependencies. In this case, the easy solution may be simply to merge TestConfiguration and TestConversion into a single test. Most of TestTicking is fine, until the very last EXPECT_EQ that references epoch_calendar. So, maybe merge them all into a single sequence?

{
const auto now = clock_type::now();
EXPECT_EQ(clock_type::from_time_t(epoch_unix), now);
EXPECT_EQ(clock_type::from_calendar(epoch_calendar), now);
EXPECT_EQ(now, clock_type::time_point{});
EXPECT_EQ(clock_type::to_time_t(now), epoch_unix);
EXPECT_EQ(clock_type::to_calendar(now), epoch_calendar);
}

TEST(SimulationClockTest, TestTicking)
{
const auto now = clock_type::now();
auto forward = std::chrono::seconds{1};
ASSERT_THROW(clock_type::tick(-1), std::domain_error);
ASSERT_NO_THROW(clock_type::tick(1));
EXPECT_EQ(clock_type::now(), now + forward);

forward += std::chrono::minutes{5};
ASSERT_THROW(clock_type::tick(std::chrono::minutes{-5}), std::domain_error);
ASSERT_NO_THROW(clock_type::tick(std::chrono::minutes{5}));
EXPECT_EQ(clock_type::now(), now + forward);

EXPECT_EQ(
clock_type::to_calendar(clock_type::now()),
epoch_calendar + forward
);
}
Loading