From 2a031870573e5793fc648a9706bfd2e5de79a6c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Wed, 7 Aug 2024 17:48:03 +0200 Subject: [PATCH 1/3] Make shape an optional attribute for constant components --- include/openPMD/backend/Attributable.hpp | 27 +++++++ src/Iteration.cpp | 3 +- src/ParticleSpecies.cpp | 3 +- src/RecordComponent.cpp | 97 +++++++++++++++++------- src/backend/PatchRecordComponent.cpp | 20 ++++- 5 files changed, 113 insertions(+), 37 deletions(-) diff --git a/include/openPMD/backend/Attributable.hpp b/include/openPMD/backend/Attributable.hpp index 0f7b722ae5..d923b83ee9 100644 --- a/include/openPMD/backend/Attributable.hpp +++ b/include/openPMD/backend/Attributable.hpp @@ -20,6 +20,7 @@ */ #pragma once +#include "openPMD/Error.hpp" #include "openPMD/IO/AbstractIOHandler.hpp" #include "openPMD/ThrowError.hpp" #include "openPMD/auxiliary/OutOfRangeMsg.hpp" @@ -30,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -113,6 +115,31 @@ namespace internal return res; } + inline auto attributes() -> A_MAP & + { + return m_attributes; + } + [[nodiscard]] inline auto attributes() const -> A_MAP const & + { + return m_attributes; + } + [[nodiscard]] inline auto + readAttribute(std::string const &name) const -> Attribute const & + { + if (auto it = m_attributes.find(name); it != m_attributes.end()) + { + return it->second; + } + else + { + throw error::ReadError( + error::AffectedObject::Attribute, + error::Reason::NotFound, + std::nullopt, + "Not found: '" + name + "'."); + } + } + private: /** * The attributes defined by this Attributable. diff --git a/src/Iteration.cpp b/src/Iteration.cpp index 366fea0de1..1afc97c3bf 100644 --- a/src/Iteration.cpp +++ b/src/Iteration.cpp @@ -595,8 +595,7 @@ void Iteration::readMeshes(std::string const &meshesPath) auto att_begin = aList.attributes->begin(); auto att_end = aList.attributes->end(); auto value = std::find(att_begin, att_end, "value"); - auto shape = std::find(att_begin, att_end, "shape"); - if (value != att_end && shape != att_end) + if (value != att_end) { MeshRecordComponent &mrc = m; IOHandler()->enqueue(IOTask(&mrc, pOpen)); diff --git a/src/ParticleSpecies.cpp b/src/ParticleSpecies.cpp index 4006cc82ba..81e7cdcd8c 100644 --- a/src/ParticleSpecies.cpp +++ b/src/ParticleSpecies.cpp @@ -76,8 +76,7 @@ void ParticleSpecies::read() auto att_begin = aList.attributes->begin(); auto att_end = aList.attributes->end(); auto value = std::find(att_begin, att_end, "value"); - auto shape = std::find(att_begin, att_end, "shape"); - if (value != att_end && shape != att_end) + if (value != att_end) { RecordComponent &rc = r; IOHandler()->enqueue(IOTask(&rc, pOpen)); diff --git a/src/RecordComponent.cpp b/src/RecordComponent.cpp index 0387268514..aef392a2fe 100644 --- a/src/RecordComponent.cpp +++ b/src/RecordComponent.cpp @@ -109,10 +109,19 @@ RecordComponent &RecordComponent::resetDataset(Dataset d) throw error::WrongAPIUsage( "[RecordComponent] Must set specific datatype."); } - // if( d.extent.empty() ) - // throw std::runtime_error("Dataset extent must be at least 1D."); + if (d.extent.empty()) + throw std::runtime_error("Dataset extent must be at least 1D."); if (d.empty()) + { + if (d.extent.empty()) + { + throw error::Internal( + "A zero-dimensional dataset is not to be considered empty, but " + "undefined. This is an internal safeguard against future " + "changes that might not consider this."); + } return makeEmpty(std::move(d)); + } rc.m_isEmpty = false; if (written()) @@ -275,6 +284,13 @@ void RecordComponent::flush( { setUnitSI(1); } + auto constant_component_write_shape = [&]() { + auto extent = getExtent(); + return !extent.empty() && + std::none_of(extent.begin(), extent.end(), [](auto val) { + return val == Dataset::JOINED_DIMENSION; + }); + }; if (!written()) { if (constant()) @@ -294,16 +310,20 @@ void RecordComponent::flush( Operation::WRITE_ATT>::ChangesOverSteps::IfPossible; } IOHandler()->enqueue(IOTask(this, aWrite)); - aWrite.name = "shape"; - Attribute a(getExtent()); - aWrite.dtype = a.dtype; - aWrite.resource = a.getResource(); - if (isVBased) + if (constant_component_write_shape()) { - aWrite.changesOverSteps = Parameter< - Operation::WRITE_ATT>::ChangesOverSteps::IfPossible; + + aWrite.name = "shape"; + Attribute a(getExtent()); + aWrite.dtype = a.dtype; + aWrite.resource = a.getResource(); + if (isVBased) + { + aWrite.changesOverSteps = Parameter< + Operation::WRITE_ATT>::ChangesOverSteps::IfPossible; + } + IOHandler()->enqueue(IOTask(this, aWrite)); } - IOHandler()->enqueue(IOTask(this, aWrite)); } else { @@ -321,6 +341,13 @@ void RecordComponent::flush( { if (constant()) { + if (!constant_component_write_shape()) + { + throw error::WrongAPIUsage( + "Extended constant component from a previous shape to " + "one that cannot be written (empty or with joined " + "dimension)."); + } bool isVBased = retrieveSeries().iterationEncoding() == IterationEncoding::variableBased; Parameter aWrite; @@ -385,28 +412,35 @@ namespace }; } // namespace +inline void breakpoint() +{} + void RecordComponent::readBase(bool require_unit_si) { using DT = Datatype; - // auto & rc = get(); - Parameter aRead; + auto &rc = get(); - if (constant() && !empty()) - { - aRead.name = "value"; - IOHandler()->enqueue(IOTask(this, aRead)); - IOHandler()->flush(internal::defaultFlushParams); + readAttributes(ReadMode::FullyReread); - Attribute a(*aRead.resource); - DT dtype = *aRead.dtype; + auto read_constant = + [&]() // comment for forcing clang-format into putting a newline here + { + Attribute a = rc.readAttribute("value"); + DT dtype = a.dtype; setWritten(false, Attributable::EnqueueAsynchronously::No); switchNonVectorType(dtype, *this, a); setWritten(true, Attributable::EnqueueAsynchronously::No); - aRead.name = "shape"; - IOHandler()->enqueue(IOTask(this, aRead)); - IOHandler()->flush(internal::defaultFlushParams); - a = Attribute(*aRead.resource); + if (!containsAttribute("shape")) + { + setWritten(false, Attributable::EnqueueAsynchronously::No); + resetDataset(Dataset(dtype, {})); + setWritten(true, Attributable::EnqueueAsynchronously::No); + + return; + } + + a = rc.attributes().at("shape"); Extent e; // uint64_t check @@ -416,7 +450,7 @@ void RecordComponent::readBase(bool require_unit_si) else { std::ostringstream oss; - oss << "Unexpected datatype (" << *aRead.dtype + oss << "Unexpected datatype (" << a.dtype << ") for attribute 'shape' (" << determineDatatype() << " aka uint64_t)"; throw error::ReadError( @@ -429,9 +463,13 @@ void RecordComponent::readBase(bool require_unit_si) setWritten(false, Attributable::EnqueueAsynchronously::No); resetDataset(Dataset(dtype, e)); setWritten(true, Attributable::EnqueueAsynchronously::No); - } + }; - readAttributes(ReadMode::FullyReread); + if (constant() && !empty()) + { + breakpoint(); + read_constant(); + } if (require_unit_si) { @@ -445,7 +483,8 @@ void RecordComponent::readBase(bool require_unit_si) "'" + myPath().openPMDPath() + "'."); } - if (!getAttribute("unitSI").getOptional().has_value()) + if (auto attr = getAttribute("unitSI"); + !attr.getOptional().has_value()) { throw error::ReadError( error::AffectedObject::Attribute, @@ -453,8 +492,8 @@ void RecordComponent::readBase(bool require_unit_si) {}, "Unexpected Attribute datatype for 'unitSI' (expected double, " "found " + - datatypeToString(Attribute(*aRead.resource).dtype) + - ") in '" + myPath().openPMDPath() + "'."); + datatypeToString(attr.dtype) + ") in '" + + myPath().openPMDPath() + "'."); } } } diff --git a/src/backend/PatchRecordComponent.cpp b/src/backend/PatchRecordComponent.cpp index af19923fad..14f42c0161 100644 --- a/src/backend/PatchRecordComponent.cpp +++ b/src/backend/PatchRecordComponent.cpp @@ -37,14 +37,26 @@ PatchRecordComponent &PatchRecordComponent::setUnitSI(double usi) PatchRecordComponent &PatchRecordComponent::resetDataset(Dataset d) { if (written()) + { throw std::runtime_error( "A Records Dataset can not (yet) be changed after it has been " "written."); - if (d.extent.empty()) - throw std::runtime_error("Dataset extent must be at least 1D."); + } if (d.empty()) - throw std::runtime_error( - "Dataset extent must not be zero in any dimension."); + { + if (d.extent.empty()) + { + throw error::Internal( + "A zero-dimensional dataset is not to be considered empty, but " + "undefined. This is an internal safeguard against future " + "changes that might not consider this."); + } + else + { + throw std::runtime_error( + "Dataset extent must not be zero in any dimension."); + } + } get().m_dataset = std::move(d); setDirty(true); From 11425ce9ff9b125627d5ad291b7da600df57a7b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Franz=20P=C3=B6schel?= Date: Fri, 11 Oct 2024 13:34:05 +0200 Subject: [PATCH 2/3] Adhere to the standard changes more closely --- include/openPMD/RecordComponent.hpp | 3 ++- .../openPMD/backend/MeshRecordComponent.hpp | 3 ++- src/Iteration.cpp | 6 +++++- src/Mesh.cpp | 17 +++++++++++------ src/ParticleSpecies.cpp | 2 ++ src/Record.cpp | 17 +++++++++++------ src/RecordComponent.cpp | 19 ++++++++++++++++++- src/backend/MeshRecordComponent.cpp | 6 ++++-- src/backend/PatchRecord.cpp | 6 +++--- 9 files changed, 58 insertions(+), 21 deletions(-) diff --git a/include/openPMD/RecordComponent.hpp b/include/openPMD/RecordComponent.hpp index ebb5a80ca8..253194d2cc 100644 --- a/include/openPMD/RecordComponent.hpp +++ b/include/openPMD/RecordComponent.hpp @@ -488,7 +488,8 @@ class RecordComponent : public BaseRecordComponent static constexpr char const *const SCALAR = "\vScalar"; protected: - void flush(std::string const &, internal::FlushParams const &); + void + flush(std::string const &, internal::FlushParams const &, bool is_scalar); void read(bool require_unit_si); private: diff --git a/include/openPMD/backend/MeshRecordComponent.hpp b/include/openPMD/backend/MeshRecordComponent.hpp index d05163d754..f8229635ba 100644 --- a/include/openPMD/backend/MeshRecordComponent.hpp +++ b/include/openPMD/backend/MeshRecordComponent.hpp @@ -47,7 +47,8 @@ class MeshRecordComponent : public RecordComponent MeshRecordComponent(); MeshRecordComponent(NoInit); void read(); - void flush(std::string const &, internal::FlushParams const &); + void + flush(std::string const &, internal::FlushParams const &, bool is_scalar); public: ~MeshRecordComponent() override = default; diff --git a/src/Iteration.cpp b/src/Iteration.cpp index 1afc97c3bf..9ef4014b5d 100644 --- a/src/Iteration.cpp +++ b/src/Iteration.cpp @@ -592,10 +592,14 @@ void Iteration::readMeshes(std::string const &meshesPath) IOHandler()->enqueue(IOTask(&m, aList)); IOHandler()->flush(internal::defaultFlushParams); + // Find constant scalar meshes. shape generally required for meshes, + // shape also required for scalars. + // https://github.com/openPMD/openPMD-standard/pull/289 auto att_begin = aList.attributes->begin(); auto att_end = aList.attributes->end(); auto value = std::find(att_begin, att_end, "value"); - if (value != att_end) + auto shape = std::find(att_begin, att_end, "shape"); + if (value != att_end && shape != att_end) { MeshRecordComponent &mrc = m; IOHandler()->enqueue(IOTask(&mrc, pOpen)); diff --git a/src/Mesh.cpp b/src/Mesh.cpp index f977bbe905..5c288001ea 100644 --- a/src/Mesh.cpp +++ b/src/Mesh.cpp @@ -222,12 +222,14 @@ void Mesh::flush_impl( auto &m = get(); if (m.m_datasetDefined) { - T_RecordComponent::flush(SCALAR, flushParams); + T_RecordComponent::flush( + SCALAR, flushParams, /* is_scalar = */ true); } else { for (auto &comp : *this) - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } else @@ -237,7 +239,7 @@ void Mesh::flush_impl( if (scalar()) { MeshRecordComponent &mrc = *this; - mrc.flush(name, flushParams); + mrc.flush(name, flushParams, /* is_scalar = */ true); } else { @@ -247,7 +249,8 @@ void Mesh::flush_impl( for (auto &comp : *this) { comp.second.parent() = &this->writable(); - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } } @@ -255,12 +258,14 @@ void Mesh::flush_impl( { if (scalar()) { - T_RecordComponent::flush(name, flushParams); + T_RecordComponent::flush( + name, flushParams, /* is_scalar = */ true); } else { for (auto &comp : *this) - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } flushAttributes(flushParams); diff --git a/src/ParticleSpecies.cpp b/src/ParticleSpecies.cpp index 81e7cdcd8c..1c3da720ce 100644 --- a/src/ParticleSpecies.cpp +++ b/src/ParticleSpecies.cpp @@ -76,6 +76,8 @@ void ParticleSpecies::read() auto att_begin = aList.attributes->begin(); auto att_end = aList.attributes->end(); auto value = std::find(att_begin, att_end, "value"); + // @todo see this comment: + // https://github.com/openPMD/openPMD-standard/pull/289#issuecomment-2407263974 if (value != att_end) { RecordComponent &rc = r; diff --git a/src/Record.cpp b/src/Record.cpp index 3bcac4d7e1..c271f03316 100644 --- a/src/Record.cpp +++ b/src/Record.cpp @@ -50,12 +50,14 @@ void Record::flush_impl( { if (scalar()) { - T_RecordComponent::flush(SCALAR, flushParams); + T_RecordComponent::flush( + SCALAR, flushParams, /* is_scalar = */ true); } else { for (auto &comp : *this) - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } else @@ -65,7 +67,7 @@ void Record::flush_impl( if (scalar()) { RecordComponent &rc = *this; - rc.flush(name, flushParams); + rc.flush(name, flushParams, /* is_scalar = */ true); } else { @@ -75,7 +77,8 @@ void Record::flush_impl( for (auto &comp : *this) { comp.second.parent() = getWritable(this); - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } } @@ -84,12 +87,14 @@ void Record::flush_impl( if (scalar()) { - T_RecordComponent::flush(name, flushParams); + T_RecordComponent::flush( + name, flushParams, /* is_scalar = */ true); } else { for (auto &comp : *this) - comp.second.flush(comp.first, flushParams); + comp.second.flush( + comp.first, flushParams, /* is_scalar = */ false); } } diff --git a/src/RecordComponent.cpp b/src/RecordComponent.cpp index aef392a2fe..5b060fab33 100644 --- a/src/RecordComponent.cpp +++ b/src/RecordComponent.cpp @@ -241,7 +241,9 @@ bool RecordComponent::empty() const } void RecordComponent::flush( - std::string const &name, internal::FlushParams const &flushParams) + std::string const &name, + internal::FlushParams const &flushParams, + bool is_scalar) { auto &rc = get(); if (flushParams.flushLevel == FlushLevel::SkeletonOnly) @@ -285,6 +287,21 @@ void RecordComponent::flush( setUnitSI(1); } auto constant_component_write_shape = [&]() { + if (is_scalar) + { + // Must write shape in any case: + // 1. Non-scalar constant components can be distinguished from + // normal components by checking if the backend reports a + // group or a dataset. This does not work for scalar constant + // components, so the parser needs to check if the attributes + // value and shape are there. If they're not, the group is + // not considered as a constant component. + // 2. Scalar constant components are required to write the shape + // by standard anyway since the standard requires that at + // least one component in a record have a shape. For scalars, + // there is only one component, so it must have a shape. + return true; + } auto extent = getExtent(); return !extent.empty() && std::none_of(extent.begin(), extent.end(), [](auto val) { diff --git a/src/backend/MeshRecordComponent.cpp b/src/backend/MeshRecordComponent.cpp index ed50080757..af2e683eb5 100644 --- a/src/backend/MeshRecordComponent.cpp +++ b/src/backend/MeshRecordComponent.cpp @@ -68,14 +68,16 @@ void MeshRecordComponent::read() } void MeshRecordComponent::flush( - std::string const &name, internal::FlushParams const ¶ms) + std::string const &name, + internal::FlushParams const ¶ms, + bool is_scalar) { if (access::write(IOHandler()->m_frontendAccess) && !containsAttribute("position")) { setPosition(std::vector{0}); } - RecordComponent::flush(name, params); + RecordComponent::flush(name, params, is_scalar); } template diff --git a/src/backend/PatchRecord.cpp b/src/backend/PatchRecord.cpp index 5d2b38d50f..2b0baf6db1 100644 --- a/src/backend/PatchRecord.cpp +++ b/src/backend/PatchRecord.cpp @@ -41,17 +41,17 @@ PatchRecord::setUnitDimension(std::map const &udim) void PatchRecord::flush_impl( std::string const &path, internal::FlushParams const &flushParams) { - if (!this->datasetDefined()) + if (!this->scalar()) { if (IOHandler()->m_frontendAccess != Access::READ_ONLY) Container::flush( path, flushParams); // warning (clang-tidy-10): // bugprone-parent-virtual-call for (auto &comp : *this) - comp.second.flush(comp.first, flushParams); + comp.second.flush(comp.first, flushParams, /* is_scalar = */ false); } else - T_RecordComponent::flush(path, flushParams); + T_RecordComponent::flush(path, flushParams, /* is_scalar = */ true); if (flushParams.flushLevel != FlushLevel::SkeletonOnly) { setDirty(false); From 3764e0113594d218ee7045d18e0cf042aeee77fe Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 11 Oct 2024 15:08:21 +0000 Subject: [PATCH 3/3] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- include/openPMD/backend/Attributable.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/openPMD/backend/Attributable.hpp b/include/openPMD/backend/Attributable.hpp index d923b83ee9..2be85a98c1 100644 --- a/include/openPMD/backend/Attributable.hpp +++ b/include/openPMD/backend/Attributable.hpp @@ -123,8 +123,8 @@ namespace internal { return m_attributes; } - [[nodiscard]] inline auto - readAttribute(std::string const &name) const -> Attribute const & + [[nodiscard]] inline auto readAttribute(std::string const &name) const + -> Attribute const & { if (auto it = m_attributes.find(name); it != m_attributes.end()) {