Skip to content

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
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch from b11eb02 to c8e46c4 Compare May 19, 2022 13:08
@@ -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 franzpoeschel force-pushed the add-attribute-getoptional branch from 0808dec to 4c56fb1 Compare July 19, 2022 09:50
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 from 4c56fb1 to 44bef9d Compare July 29, 2022 09:52
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch 2 times, most recently from bb3516f to 21edcaf Compare August 17, 2022 09:19
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch from 21edcaf to a7ac0a3 Compare October 12, 2022 09:22
Useful for backends that might not report back the right numerical type
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch from d00215f to 5d32e04 Compare October 26, 2022 10:35
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.
@franzpoeschel franzpoeschel force-pushed the add-attribute-getoptional branch from 5d32e04 to 87868d3 Compare October 26, 2022 12:36
@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