Skip to content

Conversation

@ax3l
Copy link
Member

@ax3l ax3l commented Jul 31, 2025

  • setup.py
  • CI coverage
  • fix runtime issues in tests

As a follow-up, one could add support for SP .to_df() by removing binary columns (members) from the AoS before passing it into Pandas. But this legacy layout is not our need or focus, nor worth spending more time on.

@ax3l ax3l added bug Something isn't working bug: affects latest release Bug also exists in latest release version labels Jul 31, 2025
ax3l added 4 commits July 31, 2025 12:28
And annotate harder issues in AoS.
SP has some detailed padding before `idcpu` and at the end
of each particle, if they do not align with the size of `double`.
Importing into pandas does not support binary fields, but
there is no quick way to remove them.
# print('particle 2 from aos:\n',aos[1])
# print('array interface\n', aos.__array_interface__)
arr = aos.to_numpy()
int_arg = 7 if arr[0][0].dtype == "float32" else 6 # padding
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that users will have to know, that there might be this extra padding element so the indices for the integers might be shifted?

Copy link
Member Author

@ax3l ax3l Aug 1, 2025

Choose a reason for hiding this comment

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

I fear so... I have not found a way to kick out numpy struct elements (in the SP case: the paddings) without doing a copy yet.

That said, nobody should use AoS anymore in AMReX. I am inclined to drop all support for it in pyAMReX to stop wasting time :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Solicited feedback now on #459 and on Slack

Comment on lines +74 to +77
}/*
if constexpr (ParticleType::NInt % 2 != 0) { // alignas
descr.append(py::make_tuple("", "|V4")); // empty space due to alignment
}*/
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this end-of-particle padding after the int (32bit) array fails both the double precision tests o.0

@ax3l ax3l merged commit cd2ddac into AMReX-Codes:development Aug 1, 2025
18 checks passed
@ax3l ax3l deleted the topic-sp branch August 1, 2025 20:20
@ax3l
Copy link
Member Author

ax3l commented Sep 17, 2025

There seems to be a follow-up issue in SP in ImpactX (which uses pure SoA particle layout) for .to_df():
conda-forge/impactx-feedstock#56 (comment)
Issue is a segfault with SoA_np in https://github.com/AMReX-Codes/pyamrex/blob/25.09/src/amrex/extensions/StructOfArrays.py#L41 https://github.com/AMReX-Codes/pyamrex/blob/25.09/src/amrex/extensions/PODVector.py#L35

Note the PR here just works around AoS stuff (legacy).

ax3l added a commit that referenced this pull request Sep 18, 2025
For AoS and SoA `.to_numpy()` and dependent logic
like `.to_df()`, the lifetime of the temporary copied variable is only
handled by NumPy if we first create a named variable.

The `to_host()` returned object is a temporary, and `np.array` using the
`__array_interface__` protocol does not keep it alive automatically
unless it is stored in an actual variable (eg., `tmp`).

X-ref:
-
conda-forge/impactx-feedstock#56 (comment)
-
#458 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug: affects latest release Bug also exists in latest release version bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants