Simplify volume actions, add dependency to interp framework #6623
Conversation
| const ArrayIndex& array_index, | ||
| const TemporalId& temporal_id) { | ||
| const TemporalId& temporal_id, | ||
| std::optional<std::string>&& dependency) { |
| void flag_temporal_id_as_pending(const gsl::not_null<db::DataBox<DbTags>*> box, | ||
| const TemporalId& temporal_id) { | ||
| const TemporalId& temporal_id, | ||
| std::optional<std::string>&& dependency) { |
| const typename Metavariables::InterpolatorTargetA::temporal_id::type& | ||
| /*temporal_id*/) { | ||
| /*temporal_id*/, | ||
| std::optional<std::string>&& dependency) { |
|
@wthrowe I squashed directly since it was small |
| VolumeDataAtObsId received_volume_data) { | ||
| // The below gymnastics with pointers is done in order to minimize the | ||
| // time spent locking the entire node, which is necessary because the | ||
| // DataBox does not allow any functions calls, both get and mutate, during |
There was a problem hiding this comment.
The code already exists and works, why do this change at all? Moves can be expensive, there's nothing that guarantees they won't. Moves can even fall back to copies.
There was a problem hiding this comment.
In #6624, I add a new threaded action that adds the dependency to the ObserverWriter. Since this can edit/use the same box quantities as ContributeVolumeDataToWriter we have to keep them consistent somehow. The easiest way I saw to do that was to just grab the node lock for both so that everything is guaranteed to be consistent. Along with the likelihood that we aren't writing volume data that often during a simulation compared to the amount of work being done, the simplicity of the node lock seemed a better option than the (comparatively) small performance gain we'd get having all these different locks.
There was a problem hiding this comment.
Okay, reasonable. If it shows up on profiles we can change it. Thanks for the explanation!
|
Needs a rebase. Also, now that I think of it, if we're not using the volume data lock anymore it would be good to remove it from the initialization action. |
|
@wthrowe Rebased and I removed the tag for the volume lock pointer altogether since that was the only place it was used (squashed into last commit) |
|
Did you forget to add the file? I don't see any changes to |
|
Oops, only searched the repo for |
wthrowe
left a comment
There was a problem hiding this comment.
The clang-tidy failure is a reasonable complaint. Either changing it or marking NOLINT would be fine with me.
The AddTemporalIdsToInterpolationTarget action now can also pass along a dependency for each temporal id which is added to a tag in the box. Currently nothing uses this new tag, and the temporal id is erased after the interpolation is finished.
We mostly just move volume data around so the complicated pointers seem pretty unnecessary. Keep the pointer for the H5 file lock since writing to disk is very slow.
|
@wthrowe I added a NOLINT |
Proposed changes
This is the second of 3 PRs that avoid writing volume data for a common horizon find that fails.
This PR is some miscellaneous changes to
VolumeActions.hppthat are needed and also adds what is called a "dependency" to the interpolation framework which the combined common horizon event in #6622 will make use of eventually. Currently, this dependency does nothing.Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments
Technically depends on #6622, but only for a single test so I didn't include those commits here. Whichever is merged first, I'll just rebase.