Implement simulation_clock type
#881
Conversation
…recondition assertions to conversion functions
|
Out of curiosity, did you pick up the term "controlled clock" from somewhere in particular? It seems apt, and sounds like a standard-ese-ish coinage, I've just never encountered it before. A quick Google search didn't turn it up. |
No I just made that up 😄 |
| EXPECT_EQ(clock_type::epoch(), epoch_calendar); | ||
| } | ||
|
|
||
| TEST(SimulationClockTest, TestConversion) |
There was a problem hiding this comment.
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?
| //! provides conversion utilities between simulation ticks and | ||
| //! calendar time. | ||
| //! | ||
| //! Implements named requirements: Clock (https://en.cppreference.com/w/cpp/named_req/Clock) |
There was a problem hiding this comment.
Clock or TrivialClock? This disagrees with the PR description.
There was a problem hiding this comment.
Both, though the intention is to implement Clock -- it just happens to also implement TrivialClock.
There was a problem hiding this comment.
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.
| inline simulation_clock::time_point& simulation_clock::internal_now() | ||
| { | ||
| static simulation_time_point now{}; // Default initialize to zero value | ||
| return now; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh good catch, yeah this might be incorrect. No actual reason for it being header-only, haha
| { | ||
| /* Clock named requirements */ | ||
|
|
||
| using rep = std::int64_t; // TODO: double vs std::int64_t |
There was a problem hiding this comment.
I don't think we're likely to run into sub-second time precision, so int64_t is probably fine.
| static constexpr bool is_steady = false; | ||
|
|
||
| //! Current Simulation Time | ||
| //! \returns The current point in time the simulation is at. |
There was a problem hiding this comment.
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()"
| //! \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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| return simulation_clock::internal_now(); | ||
| } | ||
|
|
||
| inline simulation_clock::time_point simulation_clock::from_calendar(const calendar_time_point& calendar_time) |
There was a problem hiding this comment.
| 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) |
This PR closes #858 by implementing a
simulation_clocktype that implements the Clock named requirement (and also happens to implement the stricter TrivialClock). However, unlike the conventionalstd::chronoclocks, this implementation is a controlled clock meaning thatsimulation_clock::now()does not change unlesssimulation_clock::tick()is called.Additions
ngen::simulation_clocktype.Checklist