Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[WIP] Unify Random-Access API and Streaming API into Series::snapshots() #1592

Open
wants to merge 78 commits into
base: dev
Choose a base branch
from
Open

[WIP] Unify Random-Access API and Streaming API into Series::snapshots() #1592

wants to merge 78 commits into from

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Jan 29, 2024 •
edited

description soon

TODO:

  • Merge Filename extensions: Allow specifying wildcards #1584 first
  • Better logic for READ_WRITE mode
  • backwards compatibility: using definitions, empty headers
  • documentation, change examples
  • maybe snapshots(Snapshots::RandomAccess) vs snapshots(Snapshots::CollectiveAccess) or so
  • Container API: How to deal with erased Iterations?
  • snapshots().begin() -> reset to start, add sth like snapshots().current()
  • Follow-up: Flush open iterations instead of dirty iterations.

Diff: franzpoeschel/openPMD-api@topic-wildcard-extension...topic-iterator

@franzpoeschel franzpoeschel marked this pull request as draft January 29, 2024 17:05
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 30, 2024
View reviewed changes
src/Series.cpp Fixed Show fixed Hide fixed
@franzpoeschel franzpoeschel force-pushed the topic-iterator branch 2 times, most recently from 1e862b7 to ab0991d Compare February 14, 2024 16:29
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 14, 2024
View reviewed changes
src/snapshots/StatefulIterator.cpp Outdated
Comment on lines 100 to 261
switch (series.iterationEncoding())
{
using IE = IterationEncoding;
case IE::fileBased:
series.readFileBased();
break;
case IE::groupBased:
case IE::variableBased: {
Parameter<Operation::OPEN_FILE> fOpen;
fOpen.name = series.get().m_name;
series.IOHandler()->enqueue(IOTask(&series, fOpen));
series.IOHandler()->flush(internal::defaultFlushParams);
using PP = Parameter<Operation::OPEN_FILE>::ParsePreference;
switch (*fOpen.out_parsePreference)
{
case PP::PerStep:
series.advance(AdvanceMode::BEGINSTEP);
series.readGorVBased(
/* do_always_throw_errors = */ false, /* init = */ true);
break;
case PP::UpFront:
series.readGorVBased(
/* do_always_throw_errors = */ false, /* init = */ true);
/*
* In linear read mode (where no parsing at all is done upon
* constructing the Series), it might turn out after parsing
* that what we expected to be a group-based Series was in fact
* a single file of a file-based Series.
* (E.g. when opening "data00000100.h5" directly instead of
* "data%T.h5")
* So we need to check the current value of
* `iterationEncoding()` once more.
*/
if (series.iterationEncoding() != IterationEncoding::fileBased)
{
series.advance(AdvanceMode::BEGINSTEP);
}
break;
}
data.parsePreference = *fOpen.out_parsePreference;
break;
}
}

Check notice

Code scanning / CodeQL

Long switch case Note

Switch has at least one case that is too long:
variableBased (38 lines)
.
include/openPMD/snapshots/StatefulIterator.hpp Fixed Show fixed Hide fixed
include/openPMD/snapshots/StatefulIterator.hpp Fixed Show fixed Hide fixed
include/openPMD/snapshots/StatefulIterator.hpp Fixed Show fixed Hide fixed
include/openPMD/snapshots/StatefulIterator.hpp Fixed Show fixed Hide fixed
@franzpoeschel franzpoeschel force-pushed the topic-iterator branch 3 times, most recently from 3845ed7 to b5e4217 Compare February 16, 2024 13:35
@franzpoeschel franzpoeschel marked this pull request as ready for review February 23, 2024 15:37
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 23, 2024
View reviewed changes
src/Series.cpp Fixed Show fixed Hide fixed
src/Series.cpp Outdated
Access at,
std::string const &options,
// Either an MPI_Comm or none, the template works for both options
MPI_Communicator &&...comm)

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable comm is not used.
test/CoreTest.cpp Fixed Show fixed Hide fixed
src/snapshots/ContainerImpls.cpp Fixed Show fixed Hide fixed
test/ParallelIOTest.cpp
@@ -753,7 +753,7 @@
REQUIRE(data[i % 4] == chunk.get()[i]);
}
auto read_again = E_x_read.loadChunk<int>({0, 0}, {mpi_size, 4});
REQUIRE_THROWS(read.flush());
// REQUIRE_THROWS(read.flush());

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@franzpoeschel franzpoeschel force-pushed the topic-iterator branch 5 times, most recently from 667ee24 to f5d8433 Compare February 29, 2024 13:32
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 5, 2024
View reviewed changes
include/openPMD/snapshots/IteratorTraits.hpp
Comment on lines +88 to +92
// increment/decrement
// ChildClass &operator++();
// ChildClass &operator--();
// ChildClass &operator++(int);
// ChildClass &operator--(int);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
include/openPMD/snapshots/IteratorTraits.hpp
Comment on lines +82 to +84
// dereference
// value_type const &operator*() const = 0;
// value_type &operator*() = 0;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 6, 2024
View reviewed changes
test/SerialIOTest.cpp
@@ -185,6 +196,7 @@
}
// ~Series intentionally not yet called

// std::cout << "READ " << filename << std::endl;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
test/SerialIOTest.cpp
@@ -163,6 +173,7 @@
auxiliary::getEnvNum("OPENPMD_TEST_NFILES_MAX", 1030);
std::string filename =
"../samples/many_iterations/many_iterations_%T." + ext;
// std::cout << "WRITE " << filename << std::endl;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
This was referenced Mar 19, 2024
Closed
Open
@franzpoeschel
BUGFIX: modifiable attributes, maybe extract this to dev
6145e12
@franzpoeschel
Ensure that iterations are never parsed twice
c9755d6
@franzpoeschel
Move currently_available_iterations to During_t
4eca444
@franzpoeschel
Revert "For now, adapt the append_mode test"
d0dfc14 
This reverts commit 19b68ee.
@franzpoeschel
Remember where we saw what iteration
e4141ef
@franzpoeschel
Bit of cleanup
3e5565c
@franzpoeschel
[wip] Groupbased writing: close and reopen
2c58566
@franzpoeschel
Further test and implement reopening of Iterations
881f297
@franzpoeschel
Unused variable
22bb4fc
@franzpoeschel
some fixes to groupbased reopen test
5ffc9ac
@franzpoeschel
Filebased reopen in ADIOS2 (no READ_WRITE support yet)
4d274ec
@franzpoeschel
Now supports READ_WRITE too in filebased mode
172b163
@franzpoeschel
Some exceptions for unimplemented stuff
7204e25
@franzpoeschel
Works in JSON and HDF5 now too
c3b4ae0
@franzpoeschel
CI fixes
bc9d2cf
@franzpoeschel
Virtual destructors
448b97c
@franzpoeschel
CI fixes continued
84cf746
@franzpoeschel
Some fixes for noexcept specifications
c298519
@franzpoeschel
Further CI Fixes
5249b39
@franzpoeschel
CI FIXES
0eb9696
@franzpoeschel
Fixes for ADIOS2 v2.7
a361279
@franzpoeschel
placate the intel compiler
d57dacc
@franzpoeschel
noexcept details for MSVC
1bc9424
@franzpoeschel
Fix ulimit test
08339b8
@franzpoeschel
Fix after rebase: dirtyRecursive
fd16ded
@franzpoeschel
Fixes after rebase
d5ad56c
@franzpoeschel franzpoeschel mentioned this pull request Apr 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: breaking breaking API changes api: new additions to the API discussion frontend: C++17 frontend: Python3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
1 participant
@franzpoeschel