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.

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

Remove necessity for RecordComponent::SCALAR #1154

Merged
merged 22 commits into from
Dec 22, 2023

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Dec 7, 2021

The last layer in the openPMD hierarchy is used for specifying different dimensions (often: x, y, z) for a dataset. In the openPMD-api:

series.iterations[0].meshes["E"]["x"]
series.iterations[0].meshes["E"]["y"]
series.iterations[0].meshes["E"]["z"]

series.iterations[0].particles["e"]["position"]["x"]
series.iterations[0].particles["e"]["position"]["y"]
series.iterations[0].particles["e"]["position"]["z"]

However, this last layer is not always needed:

series.iterations[0].meshes["e_all_chargeDensity"][RecordComponent::SCALAR]

series.iterations[0].particles["i"]["weighting"][RecordComponent::SCALAR]

The purpose of RecordComponent::SCALAR in here is to state that the last layer should be skipped. From the view of the type system, this makes sense as meshes["e_all_chargeDensity"] and meshes["e_all_chargeDensity"][RecordComponent::SCALAR] have different types.
From an API perspective however, this is an unfortunate situation and experience shows that it can be hard to explain to users. It would be better if users could directly write:

series.iterations[0].meshes["e_all_chargeDensity"]

series.iterations[0].particles["i"]["weighting"]

This feature affects the same interface components and requires many of the same changes as another planned feature: The ability to specify entirely custom datasets, e.g. series.iterations[0].customDatasets()["my"]["custom"]["hierarchy"]. Development of this PR should keep this planned feature in mind.

Done so far:

  • Make infinite Mesh chains possible
  • Maybe undo this again, I think it's not needed
  • Make RecordComponent a base class of BaseMesh

TODO:

  • Merge Apply new frontend design to Container and deriving classes #1115 first
  • Fix copy and move constructors after deriving virtually from Attributable
  • make sure that this condition is still properly checked: A scalar component can not be contained at " "the same time as one or more regular components.
  • Python bindings
  • Check that API changes in Attributable::myPath() go ok with Axel's usage (SCALAR is no longer printed out)
  • everything else
  • Having a look at the output of openpmd-pipe, there are still some unneeded attributes written by this (basis is a PIConGPU KelvinHelmholtz dataset):
    $ colordiff <(bpls -A smol_000500.bp5 | sed -E 's/ *attr//') <(bpls -A smol.bp5 | sed -E 's/ *attr//')
    >   double    /data/500/particles/e/particlePatches/numParticles/unitDimension
    >   double    /data/500/particles/e/particlePatches/numParticlesOffset/unitDimension
    >   double    /data/500/particles/i/particlePatches/numParticles/unitDimension
    >   double    /data/500/particles/i/particlePatches/numParticlesOffset/unitDimension
    

Map of the commits in this PR:

Auxiliary helpers
b71f94a Add helper: openPMD::auxiliary::overloaded

Prepare for virtual inheritance of Attributable
0435622 Prepare Attributable for virtual inheritance
e7cd246 Fix default constructors/operators of Container and BaseRecordComponent
This PR uses virtual inheritance to solve the diamond problem in order to combine the functionalities of Container<RecordComponent> (vector meshes) and RecordComponent (scalar meshes) into BaseRecord:
Bildschirmfoto vom 2023-05-05 16-06-06

Auxiliary helpers
4afb23c Add helper: openPMD::auxiliary::overloaded

Prepare for virtual inheritance of Attributable
fc20df1 Prepare Attributable for virtual inheritance
dbd07b7 Fix default constructors/operators of Container and BaseRecordComponent
This PR uses virtual inheritance to solve the diamond problem in order to combine the functionalities of Container<RecordComponent> (vector meshes) and RecordComponent (scalar meshes) into BaseRecord:
Bildschirmfoto vom 2023-05-05 16-06-06

State that will be used to track if a BaseRecord is scalar or not
d6f8057 Add m_datasetDefined

Main implementation part 1: Implement new class structure
a65eda3 Prepare class structure without applying logic yet

Main implementation part 2: Implement new logic, no longer use SCALAR
86d478e No longer use map entry SCALAR in application logic

Follow-up: Remove logic that is no longer needed, adapt tests
60b063f Remove KEEP_SYNCHRONOUS task
a98eacd Adapt Coretests

Follow-up: Properly apply Container interface to BaseRecord and some fixes for Container interface itself
f60bdac No virtual methods in Container class
10933c3 Fully override Container methods in BaseRecord
908789d Adapt Container API to C++17

Follow-up: Remove SCALAR from myPath
e692ff5 Adapt myPath() functionality

Python bindings preparation: Refactoring
8d3ff46 Factor out create_and_bind_container() function template
e616c32 Factor out RecordComponent.setitem and getitem
c792883 Consistently use copy semantics in Python API

Python bindings: Main implementation
af7dc76 Apply new class structure to Python API as well

Follow-up: Fix up openpmd-pipe (it interacted directly with openPMD types via isinstance(), so it needs to be fixed
641bf9a Adapt openpmd-pipe to new design

Follow-up: some safeguards
5bab5d0 Safeguard: No scalar and vector components side by side
d53578e Avoid object slicing when using records as scalar components

Examples and documentation
73a6c69 Remove [SCALAR] from all examples
4202b77 Documentation

Note to myself: For updating the commit map after rebasing:

cd git-repos/openPMD-api
git checkout topic-remove-scalar-component
xsel -bo | sed "$(paste <(xsel -bo | grep -E '^[[:alnum:]]{9}\>' | awk '{print $1}') <(git log --oneline --graph HEAD dev | head -n 21 | tac | awk '{print $2}') | awk '{printf "s/%s/%s/\n", $1, $2}' | tr '\n' ';')" | xsel -bi

@ax3l
Copy link
Member

ax3l commented Dec 17, 2021

@franzpoeschel ready for rebase (#1115 is in) 🎉

@ax3l
Copy link
Member

ax3l commented Feb 18, 2022

@franzpoeschel please rebase due to clang-format changes :)

You might find this helpful: pre-commit/pre-commit#1696 (comment) :)

@franzpoeschel franzpoeschel force-pushed the topic-remove-scalar-component branch 2 times, most recently from 5509b3b to f69116b Compare March 2, 2022 15:49
@ax3l
Copy link
Member

ax3l commented Apr 15, 2022

@franzpoeschel this PR seems as it needs to be merged first for a chain of other PRs like the graceful error handling.
Can you please rebase it? :)

@franzpoeschel
Copy link
Contributor Author

@ax3l This PR is not necessary for any others, but it brings in a lot of changes in our middle-end logic, so I should rebase it some time next week (and cross fingers that it will be somehow mergeable with other PRs)

@franzpoeschel
Copy link
Contributor Author

Ah, I had entered the wrong base PR in #1237

@franzpoeschel
Copy link
Contributor Author

Currently following this again on the https://github.com/franzpoeschel/openPMD-api/tree/topic-remove-scalar-component-redo branch.

Instead of managing weird inheritance hierarchies like here, that branch tries out composition over inheritance by inheriting virtually from Attributable. Let's see how that goes.

@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Oct 14, 2022

Changed to the new branch now
old branch found at https://github.com/franzpoeschel/openPMD-api/tree/topic-remove-scalar-component-old
Not everything reimplemented yet
Hitting "ready for review", so the CI can share its two cents

@franzpoeschel franzpoeschel marked this pull request as ready for review October 14, 2022 14:20
@franzpoeschel franzpoeschel force-pushed the topic-remove-scalar-component branch 2 times, most recently from d5f68d0 to 4f75a31 Compare October 21, 2022 14:10
@franzpoeschel franzpoeschel force-pushed the topic-remove-scalar-component branch 2 times, most recently from 474699a to 17ff682 Compare October 26, 2022 18:14
@franzpoeschel
Copy link
Contributor Author

franzpoeschel commented Nov 1, 2022

C++ implementation is now mostly working, Python bindings compile, but fail probably due to this issue pybind/pybind11#4303

It's an existing bug in our codebase that is triggered by this PR. In our container bindings for Python we hand out references to C++ objects:

    // keep same policy as Container class: missing keys are created
    cl.def(
        "__getitem__",
        [](Map &m, KeyType const &k) -> MappedType & { return m[k]; },
        // ref + keepalive
        py::return_value_policy::reference_internal);

After calling del series on the Python side, the referenced object is deallocated (for some reason despite the implied keepalive of reference_internal). Deregistering the handle in Python after the referenced object has been destroyed in C++ causes trouble when the referenced object has multiple/virtual inheritance in its class hierarchy.

Solution: We have shared_ptr semantics for our object model anyway, so change the return value policy to copy.
Our current Python binding semantics are a bit weird: del series should be replaced by something like series.close(), since in a garbage-collected language you cannot really rely on things such as destruction to trigger logic. It's probably possible to reestablish those semantics despite the current troubles, but we might want to take the chance and change them.

UPDATE: Now using copy semantics and propagating keepalive through the object hierarchy. Python bindings now working again.

@franzpoeschel franzpoeschel force-pushed the topic-remove-scalar-component branch 2 times, most recently from a6959ec to 5521753 Compare November 2, 2022 14:23
See in-code documentation.
BaseRecord is now derived by its contained RecordComponent type.
If it is scalar, the idea is that the BaseRecord itself is used as a
RecordComponent, without needing to retrieve the [SCALAR] entry.
No logic implemented yet around this, this just prepares the class
structure.
Notice that this will write some unnecessary attributes since the
RecordComponent types initialize some default attributes upon
construction.
Not yet supported: Backward compatibility for still allowing legacy
access to scalar entries
No longer needed, as one object in the openPMD hierarchy is no longer
represented by possibly multiple Writable objects.
Either this way, or make all of them virtual
Special care for legacy usage of SCALAR constant. Implement iteration
API such that it works for scalar components as well.
insert() and reverse iterators
Will later be called by Record-type classes, too
Similarly to the Container API, we will need to apply this to
Record-type classes.
Defining `__setitem__` and `__getitem__` for them is sufficient, as all
other members are inherited from RecordComponent.
`__setitem__` and `__getitem__` need special care, as they are inherited
from Container AND from RecordComponent, so some conflict resolution is
needed.
This somewhat demonstrates that this change is slightly API-breaking.
Since openpmd-pipe acts directly on the class structure via
`instanceof()`, fixes are necessary.
"A scalar component can not be contained at the same time as one or more regular components."
@ax3l ax3l added this to the 0.16.0 milestone Dec 7, 2023
@ax3l
Copy link
Member

ax3l commented Dec 8, 2023

Restarting CI for latest version

@ax3l ax3l closed this Dec 8, 2023
@ax3l ax3l reopened this Dec 8, 2023
@ax3l ax3l merged commit 2e89f87 into openPMD:dev Dec 22, 2023
57 checks passed
franzpoeschel added a commit to franzpoeschel/openPMD-api that referenced this pull request Jan 12, 2024
Follow-up to openPMD#1154 which changed its behavior.
franzpoeschel added a commit that referenced this pull request Jan 12, 2024
Follow-up to #1154 which changed its behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants