Skip to content

Commit 413b1c5

Browse files
franzpoeschelfranzpoeschel
authored
RecordComponent: Properly handle uninitialized datasets (#1316)
* Use a std::optional for BaseRecordComponentData::m_dataset * Fix the bugs found by this check Fix the examples, the unfinished_iteratoin_test and the Coretests * Don't flush in JSONIOHandlerImpl destructor It's not needed, and it avoids weird situations while recovering from an error. * Accept undefined dataset as long as no chunks are written yet Just ignore and skip the component in that case
1 parent 4890388 commit 413b1c5

File tree

11 files changed

+153
-58
lines changed

11 files changed

+153
-58
lines changed

‎examples/7_extended_write_serial.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,9 @@ int main()
8383
{{io::UnitDimension::M, 1}});
8484
electrons["displacement"]["x"].setUnitSI(1e-6);
8585
electrons.erase("displacement");
86-
electrons["weighting"][io::RecordComponent::SCALAR].makeConstant(
87-
1.e-5);
86+
electrons["weighting"][io::RecordComponent::SCALAR]
87+
.resetDataset({io::Datatype::FLOAT, {1}})
88+
.makeConstant(1.e-5);
8889
}
8990

9091
io::Mesh mesh = cur_it.meshes["lowRez_2D_field"];

‎examples/7_extended_write_serial.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@
9090
electrons["displacement"].unit_dimension = {Unit_Dimension.M: 1}
9191
electrons["displacement"]["x"].unit_SI = 1.e-6
9292
del electrons["displacement"]
93-
electrons["weighting"][SCALAR].make_constant(1.e-5)
93+
electrons["weighting"][SCALAR] \
94+
.reset_dataset(Dataset(np.dtype("float32"), extent=[1])) \
95+
.make_constant(1.e-5)
9496

9597
mesh = cur_it.meshes["lowRez_2D_field"]
9698
mesh.axis_labels = ["x", "y"]

‎examples/9_particle_write_serial.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@
4444
# don't like it anymore? remove it with:
4545
# del electrons["displacement"]
4646

47-
electrons["weighting"][SCALAR].make_constant(1.e-5)
47+
electrons["weighting"][SCALAR] \
48+
.reset_dataset(Dataset(np.dtype("float32"), extent=[1])) \
49+
.make_constant(1.e-5)
4850

4951
particlePos_x = np.random.rand(234).astype(np.float32)
5052
particlePos_y = np.random.rand(234).astype(np.float32)

‎include/openPMD/RecordComponent.tpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,13 @@ RecordComponent::storeChunk(Offset o, Extent e, F &&createBuffer)
332332
dCreate.name = rc.m_name;
333333
dCreate.extent = getExtent();
334334
dCreate.dtype = getDatatype();
335-
dCreate.options = rc.m_dataset.options;
335+
if (!rc.m_dataset.has_value())
336+
{
337+
throw error::WrongAPIUsage(
338+
"[RecordComponent] Must specify dataset type and extent before "
339+
"using storeChunk() (see RecordComponent::resetDataset()).");
340+
}
341+
dCreate.options = rc.m_dataset.value().options;
336342
IOHandler()->enqueue(IOTask(this, dCreate));
337343
}
338344
Parameter<Operation::GET_BUFFER_VIEW> getBufferView;

‎include/openPMD/backend/BaseRecordComponent.hpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
#include "openPMD/Error.hpp"
2525
#include "openPMD/backend/Attributable.hpp"
2626

27+
#include <optional>
28+
2729
// expose private and protected members for invasive testing
2830
#ifndef OPENPMD_protected
2931
#define OPENPMD_protected protected:
@@ -39,7 +41,7 @@ namespace internal
3941
/**
4042
* The type and extent of the dataset defined by this component.
4143
*/
42-
Dataset m_dataset{Datatype::UNDEFINED, {}};
44+
std::optional<Dataset> m_dataset;
4345
/**
4446
* True if this is defined as a constant record component as specified
4547
* in the openPMD standard.

‎src/IO/JSON/JSONIOHandlerImpl.cpp‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -58,23 +58,7 @@ JSONIOHandlerImpl::JSONIOHandlerImpl(AbstractIOHandler *handler)
5858
: AbstractIOHandlerImpl(handler)
5959
{}
6060

61-
JSONIOHandlerImpl::~JSONIOHandlerImpl()
62-
{
63-
// we must not throw in a destructor
64-
try
65-
{
66-
flush();
67-
}
68-
catch (std::exception const &ex)
69-
{
70-
std::cerr << "[~JSONIOHandlerImpl] An error occurred: " << ex.what()
71-
<< std::endl;
72-
}
73-
catch (...)
74-
{
75-
std::cerr << "[~JSONIOHandlerImpl] An error occurred." << std::endl;
76-
}
77-
}
61+
JSONIOHandlerImpl::~JSONIOHandlerImpl() = default;
7862

7963
std::future<void> JSONIOHandlerImpl::flush()
8064
{

‎src/RecordComponent.cpp‎

Lines changed: 56 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ namespace internal
4242
RecordComponent impl{
4343
std::shared_ptr<RecordComponentData>{this, [](auto const *) {}}};
4444
impl.setUnitSI(1);
45-
impl.resetDataset(Dataset(Datatype::CHAR, {1}));
4645
}
4746
} // namespace internal
4847

@@ -71,11 +70,17 @@ RecordComponent &RecordComponent::resetDataset(Dataset d)
7170
auto &rc = get();
7271
if (written())
7372
{
73+
if (!rc.m_dataset.has_value())
74+
{
75+
throw error::Internal(
76+
"Internal control flow error: Written record component must "
77+
"have defined datatype and extent.");
78+
}
7479
if (d.dtype == Datatype::UNDEFINED)
7580
{
76-
d.dtype = rc.m_dataset.dtype;
81+
d.dtype = rc.m_dataset.value().dtype;
7782
}
78-
else if (d.dtype != rc.m_dataset.dtype)
83+
else if (d.dtype != rc.m_dataset.value().dtype)
7984
{
8085
throw std::runtime_error(
8186
"Cannot change the datatype of a dataset.");
@@ -99,7 +104,7 @@ RecordComponent &RecordComponent::resetDataset(Dataset d)
99104
rc.m_isEmpty = false;
100105
if (written())
101106
{
102-
rc.m_dataset.extend(std::move(d.extent));
107+
rc.m_dataset.value().extend(std::move(d.extent));
103108
}
104109
else
105110
{
@@ -112,12 +117,28 @@ RecordComponent &RecordComponent::resetDataset(Dataset d)
112117

113118
uint8_t RecordComponent::getDimensionality() const
114119
{
115-
return get().m_dataset.rank;
120+
auto &rc = get();
121+
if (rc.m_dataset.has_value())
122+
{
123+
return rc.m_dataset.value().rank;
124+
}
125+
else
126+
{
127+
return 1;
128+
}
116129
}
117130

118131
Extent RecordComponent::getExtent() const
119132
{
120-
return get().m_dataset.extent;
133+
auto &rc = get();
134+
if (rc.m_dataset.has_value())
135+
{
136+
return rc.m_dataset.value().extent;
137+
}
138+
else
139+
{
140+
return {1};
141+
}
121142
}
122143

123144
namespace detail
@@ -149,6 +170,12 @@ RecordComponent &RecordComponent::makeEmpty(Dataset d)
149170
auto &rc = get();
150171
if (written())
151172
{
173+
if (!rc.m_dataset.has_value())
174+
{
175+
throw error::Internal(
176+
"Internal control flow error: Written record component must "
177+
"have defined datatype and extent.");
178+
}
152179
if (!constant())
153180
{
154181
throw std::runtime_error(
@@ -158,30 +185,30 @@ RecordComponent &RecordComponent::makeEmpty(Dataset d)
158185
}
159186
if (d.dtype == Datatype::UNDEFINED)
160187
{
161-
d.dtype = rc.m_dataset.dtype;
188+
d.dtype = rc.m_dataset.value().dtype;
162189
}
163-
else if (d.dtype != rc.m_dataset.dtype)
190+
else if (d.dtype != rc.m_dataset.value().dtype)
164191
{
165192
throw std::runtime_error(
166193
"Cannot change the datatype of a dataset.");
167194
}
168-
rc.m_dataset.extend(std::move(d.extent));
195+
rc.m_dataset.value().extend(std::move(d.extent));
169196
rc.m_hasBeenExtended = true;
170197
}
171198
else
172199
{
173200
rc.m_dataset = std::move(d);
174201
}
175202

176-
if (rc.m_dataset.extent.size() == 0)
203+
if (rc.m_dataset.value().extent.size() == 0)
177204
throw std::runtime_error("Dataset extent must be at least 1D.");
178205

179206
rc.m_isEmpty = true;
180207
dirty() = true;
181208
if (!written())
182209
{
183210
switchType<detail::DefaultValue<RecordComponent> >(
184-
rc.m_dataset.dtype, *this);
211+
rc.m_dataset.value().dtype, *this);
185212
}
186213
return *this;
187214
}
@@ -213,11 +240,23 @@ void RecordComponent::flush(
213240
/*
214241
* This catches when a user forgets to use resetDataset.
215242
*/
216-
if (rc.m_dataset.dtype == Datatype::UNDEFINED)
243+
if (!rc.m_dataset.has_value())
217244
{
218-
throw error::WrongAPIUsage(
219-
"[RecordComponent] Must set specific datatype (Use "
220-
"resetDataset call).");
245+
// The check for !written() is technically not needed, just
246+
// defensive programming against internal bugs that go on us.
247+
if (!written() && rc.m_chunks.empty())
248+
{
249+
// No data written yet, just accessed the object so far without
250+
// doing anything
251+
// Just do nothing and skip this record component.
252+
return;
253+
}
254+
else
255+
{
256+
throw error::WrongAPIUsage(
257+
"[RecordComponent] Must specify dataset type and extent "
258+
"before flushing (see RecordComponent::resetDataset()).");
259+
}
221260
}
222261
if (!written())
223262
{
@@ -243,7 +282,7 @@ void RecordComponent::flush(
243282
dCreate.name = name;
244283
dCreate.extent = getExtent();
245284
dCreate.dtype = getDatatype();
246-
dCreate.options = rc.m_dataset.options;
285+
dCreate.options = rc.m_dataset.value().options;
247286
IOHandler()->enqueue(IOTask(this, dCreate));
248287
}
249288
}
@@ -262,7 +301,7 @@ void RecordComponent::flush(
262301
else
263302
{
264303
Parameter<Operation::EXTEND_DATASET> pExtend;
265-
pExtend.extent = rc.m_dataset.extent;
304+
pExtend.extent = rc.m_dataset.value().extent;
266305
IOHandler()->enqueue(IOTask(this, std::move(pExtend)));
267306
rc.m_hasBeenExtended = false;
268307
}

‎src/backend/BaseRecordComponent.cpp‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,13 +35,29 @@ BaseRecordComponent &BaseRecordComponent::resetDatatype(Datatype d)
3535
"A Records Datatype can not (yet) be changed after it has been "
3636
"written.");
3737

38-
get().m_dataset.dtype = d;
38+
auto &rc = get();
39+
if (rc.m_dataset.has_value())
40+
{
41+
rc.m_dataset.value().dtype = d;
42+
}
43+
else
44+
{
45+
rc.m_dataset = Dataset{d, {1}};
46+
}
3947
return *this;
4048
}
4149

4250
Datatype BaseRecordComponent::getDatatype() const
4351
{
44-
return get().m_dataset.dtype;
52+
auto &rc = get();
53+
if (rc.m_dataset.has_value())
54+
{
55+
return rc.m_dataset.value().dtype;
56+
}
57+
else
58+
{
59+
return Datatype::UNDEFINED;
60+
}
4561
}
4662

4763
bool BaseRecordComponent::constant() const
@@ -54,8 +70,12 @@ ChunkTable BaseRecordComponent::availableChunks()
5470
auto &rc = get();
5571
if (rc.m_isConstant)
5672
{
57-
Offset offset(rc.m_dataset.extent.size(), 0);
58-
return ChunkTable{{std::move(offset), rc.m_dataset.extent}};
73+
if (!rc.m_dataset.has_value())
74+
{
75+
return ChunkTable{};
76+
}
77+
Offset offset(rc.m_dataset.value().extent.size(), 0);
78+
return ChunkTable{{std::move(offset), rc.m_dataset.value().extent}};
5979
}
6080
containingIteration().open();
6181
Parameter<Operation::AVAILABLE_CHUNKS> param;

‎src/backend/PatchRecordComponent.cpp‎

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,15 @@ uint8_t PatchRecordComponent::getDimensionality() const
6767

6868
Extent PatchRecordComponent::getExtent() const
6969
{
70-
return get().m_dataset.extent;
70+
auto &rc = get();
71+
if (rc.m_dataset.has_value())
72+
{
73+
return rc.m_dataset.value().extent;
74+
}
75+
else
76+
{
77+
return {1};
78+
}
7179
}
7280

7381
PatchRecordComponent::PatchRecordComponent() : BaseRecordComponent{nullptr}
@@ -94,13 +102,32 @@ void PatchRecordComponent::flush(
94102
}
95103
else
96104
{
105+
if (!rc.m_dataset.has_value())
106+
{
107+
// The check for !written() is technically not needed, just
108+
// defensive programming against internal bugs that go on us.
109+
if (!written() && rc.m_chunks.empty())
110+
{
111+
// No data written yet, just accessed the object so far without
112+
// doing anything
113+
// Just do nothing and skip this record component.
114+
return;
115+
}
116+
else
117+
{
118+
throw error::WrongAPIUsage(
119+
"[PatchRecordComponent] Must specify dataset type and "
120+
"extent before flushing (see "
121+
"RecordComponent::resetDataset()).");
122+
}
123+
}
97124
if (!written())
98125
{
99126
Parameter<Operation::CREATE_DATASET> dCreate;
100127
dCreate.name = name;
101128
dCreate.extent = getExtent();
102129
dCreate.dtype = getDatatype();
103-
dCreate.options = rc.m_dataset.options;
130+
dCreate.options = rc.m_dataset.value().options;
104131
IOHandler()->enqueue(IOTask(this, dCreate));
105132
}
106133

0 commit comments

Comments
 (0)