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

Add Attribute::getOptional<T>() and use to add some more dynamic datatype conversions at read time #1278

Merged
merged 3 commits into from
Nov 1, 2022

Conversation

franzpoeschel
Copy link
Contributor

Currently, we have Attribute::get<T>() that throws a std::runtime_error() if no conversion is possible.
Adding Attribute::getOptional<T>() is helpful to check if a conversion is possible without having to resort to catching exceptions.

This PR also uses that new API method while reading a Series in. This became necessary in #1277 from where I have extracted this PR (that PR adds a less verbose mode to the JSON backend that does not explicitly annotate datatypes and might hence report different numerical types at read times).

@franzpoeschel franzpoeschel added internal api: new additions to the API labels May 19, 2022
@@ -251,4 +370,16 @@ U Attribute::get() const
return getCast<U>(Variant::getResource());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove getCast here in a similar way. Slightly API-breaking though since this is in a public header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this API break now. All implementation details are now moved to the detail namespace.

@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch 4 times, most recently from be0deb0 to 003129f Compare May 20, 2022 12:27
@franzpoeschel franzpoeschel requested a review from ax3l May 23, 2022 11:17
@ax3l ax3l self-assigned this May 24, 2022
* An empty std::optional if no conversion is possible.
*/
template <typename U>
std::optional<U> getOptional() const;
Copy link
Member

@ax3l ax3l May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add in Python, too (pybind11 returns py:None for std::nullopt)

Copy link
Contributor Author

@franzpoeschel franzpoeschel May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Python does not use this call at all:

        // C++ pass-through API: Getter
        .def(
            "get_attribute",
            [](Attributable &attr, std::string const &key) {
                auto v = attr.getAttribute(key);
                return v.getResource();
                // TODO instead of returning lists, return all arrays (ndim > 0)
                // as numpy arrays?
            })

There is no explicit type requesting as get<T>() does, in Python you just get the type that there is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was wondering if we want to add such an option in Python to cast?

@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch 2 times, most recently from 8a93fad to 0808dec Compare May 30, 2022 12:53
franzpoeschel added a commit to franzpoeschel/openPMD-api that referenced this pull request Jul 21, 2022
franzpoeschel added a commit to franzpoeschel/openPMD-api that referenced this pull request Jul 22, 2022
franzpoeschel added a commit to franzpoeschel/openPMD-api that referenced this pull request Jul 29, 2022
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch 2 times, most recently from bb3516f to 21edcaf Compare August 17, 2022 09:19
Useful for backends that might not report back the right numerical type
Note that this is slightly API breaking since the shared code was in a
public header.
All implementation details are now in the detail namespace.
@ax3l ax3l added the api: breaking breaking API changes label Nov 1, 2022
@ax3l ax3l merged commit 8c8d14f into openPMD:dev Nov 1, 2022
eschnett added a commit to eschnett/openPMD-api that referenced this pull request Nov 11, 2022
* dev: (70 commits)
  Docs: Recommend Static Build for Superbuilds (openPMD#1325)
  Python 3.11 (openPMD#1323)
  pybind11: v2.10.1+ (openPMD#1322)
  Add Attribute::getOptional<T>() and use to add some more dynamic datatype conversions at read time (openPMD#1278)
  Mapping between ADIOS steps and openPMD iterations (openPMD#949)
  Deprecate shareRaw (openPMD#1229)
  Fix append mode double attributes (openPMD#1302)
  Constant scalars: Don't flush double (openPMD#1315)
  Remove caching cmake vars (openPMD#1313)
  [pre-commit.ci] pre-commit autoupdate (openPMD#1311)
  storeChunk: Add an overload for shared_ptr<T[]> (openPMD#1296)
  Fix `operationAsString` Export (openPMD#1309)
  ADIOS2: more fine-grained control for file endings (openPMD#1218)
  [pre-commit.ci] pre-commit autoupdate (openPMD#1307)
  Fix file existence check in parallel tests (openPMD#1303)
  ADIOS2: Flush to disk within a step (openPMD#1207)
  [pre-commit.ci] pre-commit autoupdate (openPMD#1304)
  [pre-commit.ci] pre-commit autoupdate (openPMD#1295)
  Update catch2 to v2.13.9 (openPMD#1299)
  [pre-commit.ci] pre-commit autoupdate (openPMD#1292)
  ...

# Conflicts:
#	.github/workflows/linux.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: breaking breaking API changes api: new additions to the API internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants