current_i: assumes zero-base & is dump number

[openpmd]

current_i: assumes zero-base & is dump number

[RemiLehe]

Thanks for pointing out a potential issue.
However, what do you mean by "assumes dump period == 1" in the title ? In the openPMD-example-datasets for instance, the dump period is 100 PIC cycles, and the openPMD-viewer can read it without problems.

I am unsure if this is what you mean, but note that current_i is never directly used as the iteration, in the code. Instead, we use self.iterations[self.current_i], since, as you mentioned, current_i represents the n-th output and not the iteration of that output.

Do you have maybe a concrete example to illustrate how the current code would fail?

[ax3l]

Ah the title is misleading, yes! I had a restarted simulation in PIConGPU that was running between steps [5'000:9'000] and only contained those outputs. At some point in the post-processing, I wanted to know "what real time is it" for a certain iteration.

The variable current_i, a publicly accessible member, is a bit ill-named (current_i: iteration, current_t: time) for the above mentioned usage. I also had no way to implement a ts.find_time(iteration) function into OpenPMDTimeSeries without reading a specific record and using its meta attributes.

[RemiLehe]

OK, thanks for the clarification.

Here are a few suggestions of what coud be changed:

• Regarding the fact that current_i is a public attribute: note that, in Python, all class attribute are public. However, we can rename it as _current_i (in Python-world, the underscore means that the attribute is intended to be private - although it is technically still public)

• Regarding the finding the time that corresponds to a given iteration, it can be done by:

t = ts.t[ ts.iterations == iteration ]

But we could also define a function find_time that does this. What do you think?

[ax3l]

Yes close, but I would either store the real iteration in current_i or rename it to current_dump/file/... because the name is confusing.

I can also provide a PR changing the content and usage of current_i at all places to correct it, but I did not want to disrupt parallel developments without prior notice via an issue.

[RemiLehe]

How about doing the following changes:

• replacing current_i by _current_i
• adding a new attribute current_iteration. This would be more consistent then current_i, since for instance the get_particle and get_field API has t and iteration as arguments (hence current_t and current_iteration)

[RemiLehe]

If you are fine with the above, you can definitely implement it and do the corresponding PR.

[ax3l]

sorry for the late reply since I work-arounded the problem and it then suddenly fall in priority behind in the end-of-year madness.

I think it's a great idea to add an explicit

• current_iteration

for naming & interface consistency.
I would suggest we rename current_i to current_dump or even better current_file_index to make it self-explanatory, too. We can still _hide it if you want.

[RemiLehe]

Thanks for the above comments. I agree with all of the above, and I slightly prefer current_file_index over current_dump.
You can go ahead with a PR if you have the time. Otherwise, I'll do it at some point later.

[RemiLehe]

Closed by #164