Allow timestep to be set when XIOS handler is created #750
Conversation
core/src/Xios.cpp
Outdated
| // Set default calendar origin and start // TODO: Pick something sensible | ||
| setCalendarOrigin(TimePoint("2020-01-23T00:08:15Z")); | ||
| setCalendarStart(TimePoint("2023-03-17T17:11:00Z")); |
There was a problem hiding this comment.
Any thoughts on suitable default calendar origin and start times?
There was a problem hiding this comment.
I guess the start will be read from the config file.
There was a problem hiding this comment.
The time classes in the rest of nextSIM use the Unix epoch as their calendar origin.
There was a problem hiding this comment.
Thanks, now addressed in 542da5b.
core/src/include/Xios.hpp
Outdated
| std::string _calendarType; | ||
| std::string _contextId; | ||
| Duration _timestep; |
There was a problem hiding this comment.
Unless this is an XIOS convention, I'd discourage leading underscores in nextSIM code, since we generally try to align with the WebKit coding standard (albeit imperfectly).
There was a problem hiding this comment.
Sure, addressed in 197d747.
core/test/XiosCalendar_test.cpp
Outdated
| REQUIRE(xios_handler.getCalendarTimestep().seconds() == doctest::Approx(3600.0)); // Default | ||
| Duration timestep("P0-0T01:30:00"); | ||
| REQUIRE(timestep.seconds() == doctest::Approx(5400.0)); |
There was a problem hiding this comment.
I know that testing floating point values can be imprecise, but I'd suggest that if we don't get back the exact integer values of 3600 and 5400, that should also be a test failure.
There was a problem hiding this comment.
Good point - fixed in 2eec43f.
|
I also provided the option to set the calendar start upon creating an |
jwallwork23
force-pushed
the
init-timestep
branch
from
10e4955 to
d5b28d0
Compare
|
@timspainNERSC please could you re-review this? I think I addressed all your concerns. |
.pre-commit
Outdated
There was a problem hiding this comment.
Do we want to merge these changes as part of this PR, rather than in a separate PR?
There was a problem hiding this comment.
I'd figured it was a very small change so didn't know if it warranted it's own PR. But yeah, it'd be better practice to put it in a separate PR. I'll do that now.
Thanks @timspainNERSC! |
jwallwork23
force-pushed
the
init-timestep
branch
from
0eae60a to
cd35f66
Compare
Refinement of #751
Allow timestep to be set when XIOS handler is created
This PR makes a few minor modifications to the
Xioshandler object:Xiosinstance.These steps should make it easier to set up XIOS and reduce the amount of additional boilerplate when using it to drive I/O rather than the existing approach.