Skip to content

attempt to cache the index and avoid some indexOf() call #1860

Merged
franzpoeschel merged 9 commits intoopenPMD:devfrom
guj:issue1859
Mar 10, 2026
Merged

attempt to cache the index and avoid some indexOf() call #1860
franzpoeschel merged 9 commits intoopenPMD:devfrom
guj:issue1859

Conversation

@guj
Copy link
Contributor

@guj guj commented Mar 5, 2026

Hope this resolves issue #1859

Copy link
Contributor

@franzpoeschel franzpoeschel left a comment

Choose a reason for hiding this comment

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

Hey Junmin, thank you for the PR. Some comments below.

* If not, see <http://www.gnu.org/licenses/>.
*/

#include "openPMD/Iteration.hpp" // needed for index caching in operator[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-declaring should be fine, but note my comment below, this entire logic is better placed in linkHierarchy()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, didn't know, let me see

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 think I don't know linkHierarchy() enough to decide. It looks like a big change to me. Maybe merge this pull request and you make a new pull request and work on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I put the logic into GenerationPolicy now, linkHierarchy was not adequate upon closer looking. With this, I think we can merge.

@franzpoeschel franzpoeschel enabled auto-merge (squash) March 10, 2026 10:47
@franzpoeschel franzpoeschel merged commit c128403 into openPMD:dev Mar 10, 2026
31 checks passed
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.

3 participants