ComputationalRadiationPhysics / libSplash

libSplash - Simple Parallel file output Library for Accumulating Simulation data using Hdf5
GNU Lesser General Public License v3.0
15 stars 15 forks source link

DataContainer: Fix Offset Read #263

Closed ax3l closed 6 years ago

ax3l commented 7 years ago

Proposed fix for #254 on which the offset >0 during reads could never be determined.

@Flamefire maybe you want to take a look? I would still like to add a test for that, but did not dig deeper.

ax3l commented 7 years ago

RT test verifies that https://github.com/ComputationalRadiationPhysics/picongpu/issues/1544 is fixed with this PR

a test that failed before in libSplash itself would still be awesome

ax3l commented 7 years ago

Also shouldn't the initial size by zero instead of (1,1,1)?

I wouldn't change that for now, I don't have the internal definitions of libSplash in mind right now and it does not cause problems ;)

ax3l commented 7 years ago

@Flamefire do you have any idea on a test? I tried to reproduce the splash2txt problem above in the libSplash tests yesterday but found no way to add it (honestly, I was suprised it was not covered yet by them already... maybe because the tests don't use several subdomains in may cases...?)

Flamefire commented 7 years ago

Add a single subdomain with a non-zero offset. Should be enough to trigger the problem. A test case I'd use would be to add a subdomain with non-zero offset, than 3 more that extend the extents to either side and both sides (offset and size)

ax3l commented 7 years ago

The examples that are already checked in always use a non-zero offset and still don't trigger it 😕

Flamefire commented 7 years ago

Thats impossible as min(0, x) = 0 which makes it a bigger size and wrong offset as it should be. Then what exactly is the behaviour you'd want to see triggered?

ax3l commented 7 years ago

Ideally I want to see a == NULL of a getIndex() of a DataContainer for a non-zero offset on read, the same issue we saw in splashtxt: https://github.com/ComputationalRadiationPhysics/picongpu/issues/1544

(before this patch, this should fail)

Flamefire commented 7 years ago

getIndex is a badly named function IMO. But you probably meant getElement(). Idea which is similar to splash2txt:

After fix:

ax3l commented 6 years ago

Added a test that triggered the bug to libSplash and fixed it with the proposed fix to verify :)