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
Construct ObservationId directly from a value #1408
Construct ObservationId directly from a value #1408
Conversation
4fb16bc
to
3c3b1fb
Compare
Yes, this looks good. |
I'll review, but might not have time until Tuesday or Wednesday :) |
@wthrowe How should we handle time here? I can think of the following options:
|
f1c447b
to
29a4466
Compare
Use |
@wthrowe I made the changes, this should be ready for review now |
2b2c24a
to
f761f2e
Compare
@@ -81,11 +55,11 @@ SPECTRE_TEST_CASE("Unit.IO.Observers.ObservationId", "[Unit][Observers]") { | |||
CHECK(id4.observation_type_hash() != id3.observation_type_hash()); | |||
CHECK(id4.value() == 8.0); | |||
|
|||
ObservationId time0(Time(Slab{0.0, 1.0}, 0), | |||
ObservationId time0(Time(Slab{0.0, 1.0}, 0).value(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section (everything with the timeN
objects) is a duplicate of the preceding section now.
fixed @wthrowe. @nilsdeppe and/or @markscheel please review |
ce865c7
to
4871636
Compare
Looks good. |
4871636
to
e87d3fa
Compare
Rebased and squashed |
We don't need the wrapper anymore since ObservationId can now construct directly from a value
e87d3fa
to
7f99a89
Compare
Rebased on #1385 |
@wthrowe please look at; you previously gave the go ahead for a squash, so it should be quick; please merge if you are happy with it. |
Proposed changes
This PR changes the constructor of
ObservationId
to take adouble value
directly, instead of a type that has avalue()
member function. This allows me to remove the wrapper aroundsize_t
that I used for theLinearSolver::IterationId
. I haven't propagated the change to the other places that constructObservationId
yet, so opening this as a draft PR to confirm the design.Types of changes:
Component:
Code review checklist
clang-tidy
andIWYU
.For instructions on how to perform the CI checks locally refer to the Dev
guide on the Travis CI.
make doc
to generate the documentation locally intoBUILD_DIR/docs/html
.Then open
index.html
.code review guide.