Skip to content

Python: grid_spacing & time_offset double#1290

Merged
ax3l merged 1 commit intoopenPMD:devfrom
ax3l:python-mesh-double
Dec 20, 2022
Merged

Python: grid_spacing & time_offset double#1290
ax3l merged 1 commit intoopenPMD:devfrom
ax3l:python-mesh-double

Conversation

@ax3l
Copy link
Member

@ax3l ax3l commented Jun 8, 2022

In pybind11, overloads on types are order-dependent (first wins).
pybind/pybind11#1512

We specialize double here generically and cast in read if needed (see #345 #1137).

Later on, we could add support for 1D numpy arrays with distinct type.

In `pybind11`, overloads on types are order-dependent (first wins).
pybind/pybind11#1512

We specialize `double` here generically and cast in read if needed
(see openPMD#345 openPMD#1137).

Later on, we could add support for 1D numpy arrays with distinct
type.
@franzpoeschel
Copy link
Contributor

Is there a reason not to specify long double generically instead of double? With this PR, don't you get a narrowing conversion if the Series stores these attributes as a long double type?

@ax3l
Copy link
Member Author

ax3l commented Aug 9, 2022

So in Python there is only a dynamic-bitness builtin float type. We could use long double here to case up to the platform-specific largest type, but I think I was conservative in my design here because long double differs from platform to platform.

I think the most general thing that we could do for the getter:

  • write a lambda switch loop that checks at runtime the type of the attribute
  • return a std::variant<float, double, long double>
    and for the setter:
  • overload for generic Python double type and
  • numpy scalar

For now, this feels like a bit too detailed since there is most likely a conversation when going to Python builtin float anyways.

@franzpoeschel
Copy link
Contributor

With the current status of this PR, I think the following situation might happen:

openPMD dataset        ---> openPMD Python API               ---> Python script
(contains long double)      (uses Mesh::gridSpacing<double>)      (platform supports long double, but value was cast to double)

Your point of cross-platform consistency is valid, so I think we can go forward with this PR. We should just maybe be aware that this leaves stored long double values accessible only via C++.

@ax3l
Copy link
Member Author

ax3l commented Dec 20, 2022

Yes, that's the problem we need to generalize 👍

@ax3l ax3l merged commit babddfb into openPMD:dev Dec 20, 2022
@ax3l ax3l deleted the python-mesh-double branch December 20, 2022 10:59
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