Fix: Read Inconsistent Zero Pads

[ax3l]

Some codes mess up the zero-padding in fileBased encoding, e.g., when specifying padding to 5 digits but creating >100'000 output steps.

Files like those cannot yet be parsed using an openpmd_%T.h5 file open string and fell back to no padding, which fails to open the file:

openpmd_00000.h5
openpmd_02000.h5
openpmd_101000.h5
openpmd_01000.h5
openpmd_100000.h5
openpmd_104000.h5

Error:

RuntimeError: [HDF5] Failed to open HDF5 file diags/diag1/openpmd_0.h5

To Do

• clean up
• pass existing tests
• add new test with above pattern

[franzpoeschel]

I've reverted your changes and pushed a suggested implementation that should fix this
A quick test seems to confirm it:

In [1]: import openpmd_api as io                

In [2]: io.Series("data%T.json", io.Access.create)                                              
Out[2]: <openPMD.Attributable with '8' attributes>

In [3]: series = io.Series("data%T.json", io.Access.create)                                     
In [4]: series.iterations[3000]                 
Out[4]: <openPMD.Iteration of at t = '0.000000 s'>

In [5]: del series      

In [6]: series = io.Series("data%02T.json", io.Access.read_only)                                

In [7]: [key for key in series.iterations]      
Out[7]: [3000]

The trick was to modify the regex a bit that parses a directory's filenames
e.g. the regex for a padding of 5: ^<prefix>(([1-9][[:digit:]]*)?([[:digit:]]{5}))<postfix>$:

• The part after the question mark: Exactly five digits, these need to be given at the minimum
• The part before the question mark: Optionally, further digits, but only if the first digit is not 0

In consequence, this regex now accepts 00001, 50000, 550000, 999999999999999999, but not 001, 1 or 000050000

[ax3l]

Cool, I think that works well!

I think a general thing we could also consider is that prefixes that are weirdly named, e.g., diag1 that then read diag10004.bp can cause problems. I generally implement this in user-code with a separating underscore to avoid the ambiguity.

Should we maybe add a warning if the user-provided name does end in a number of the prefix before the %T?

[ax3l]

I pushed test bug it looks like that one passes already before the patch - as long as we use %T.

Maybe that's all that is needed in openPMD-viewer then.

[ax3l]

Ah, I think the problem was not in Series::Series but when we actually read the data and do a series.flush() on a specific iteration. Let's refine the test.

[franzpoeschel]

Should we maybe add a warning if the user-provided name does end in a number of the prefix before the %T?

Theoretically we could start soft-enforcing the underscore by giving a warning if anything but an underscore is used?

A prefix diag1 should be technically unproblematic since the 1 is a hardcoded part of the regex and will not be considered for the iteration number. But user-side confusion is very likely in such cases, yeah

I pushed test bug it looks like that one passes already before the patch - as long as we use %T.

Yep, if using %T, the regex will just match for [[digit]]+, so any number of digits will be accepted. So, %T is a good catch-all for inconsistent paddings. This could well fix your use case, but I still think that going forward with this PR is necessary. %06T should be able to read all files that it produces.

[franzpoeschel]

Theoretically we could start soft-enforcing the underscore by giving a warning if anything but an underscore is used?

I've pushed a commit that does this. In case we want to only warn if the prefix ends in a digit, that should be easy enough to fix.

Apparently, there is an instance of data%T in our test suite:

[Warning] In file-based iteration encoding, it is strongly recommended
to prepend the expansion pattern of the filename with an underscore '_'.
Example: 'data_%T.json' or 'simOutput_%06T.h5'
Given file pattern: 'data%T'

[ax3l]

Not sure if we need to warn on not ending in an underscore, ending in a digit is maybe worth a warning.

…

On November 1, 2021 8:12:24 AM PDT, "Franz Pöschel" ***@***.***> wrote: > Theoretically we could start soft-enforcing the underscore by giving a warning if anything but an underscore is used? I've pushed a commit that does this. In case we want to only warn if the prefix ends in a digit, that should be easy enough to fix. Apparently, there is an instance of `data%T` in our test suite: ``` [Warning] In file-based iteration encoding, it is strongly recommended to prepend the expansion pattern of the filename with an underscore '_'. Example: 'data_%T.json' or 'simOutput_%06T.h5' Given file pattern: 'data%T' ```

[ax3l]

I was initially thinking here: should we read 123456 if our padding was specifically asked to be 00?

But if we would skip it, then this would be a bit inconsistent with reading unpadded numbers.
Thus, I think the logic change here in the test that you added is the proper way to handle this 👍

[franzpoeschel]

should we read 123456 if our padding was specifically asked to be 00?

Yep, it should, because that number is not padded ;)

[ax3l]

@franzpoeschel, thanks a lot for your help and updates 💖

[ax3l]

CI: conda install seems to be hanging today, nothing to worry

[ax3l]

@camille12225 reports that even with this port in 0.14.3, she still sees issues with inconsistent padding in files.

I added a report to #1173