Fix Centers function

[acruzpr]

Fixes #77

Using the test.dx.gz file:

# OpenDX density file written by gridDataFormats.Grid.export()
# File format: http://opendx.sdsc.edu/docs/html/pages/usrgu068.htm#HDREDF
# Data are embedded in the header and tied to the grid positions.
# Data is written in C array order: In grid[x,y,z] the axis z is fastest
# varying, then y, then finally x, i.e. z is the innermost loop.
# (Note: the VMD dx-reader chokes on comments below this line)
object 1 class gridpositions counts 2 2 2
origin 20.100000 3.000000 -10.000000
delta 1.000000 0.000000 0.000000
delta 0.000000 1.000000 0.000000
delta 0.000000 0.000000 1.000000
object 2 class gridconnections counts 2 2 2
object 3 class array type "double" rank 0 items 8 data follows
1.000000000000000 1.000000000000000 1.000000000000000
1.000000000000000 0.000001000000000 -1000000.000000000000000
1.000000000000000 1.000000000000000
attribute "dep" string "positions"
object "density" class field
component "positions" value 1
component "connections" value 2
component "data" value 3

The coordinates of the first center are the same as the coordinates of the origin. These coordinates should be displaced by self.delta * 0.5 to identify the centers of the grid cells.

from gridData import Grid
g = Grid('test.dx')
g.origin
array([ 20.1,   3. , -10. ])

for i in g.centers():
    print(i)

[ 20.1   3.  -10. ]
[20.1  3.  -9. ]
[ 20.1   4.  -10. ]
[20.1  4.  -9. ]
[ 21.1   3.  -10. ]
[21.1  3.  -9. ]
[ 21.1   4.  -10. ]
[21.1  4.  -9. ]

After the fix:

from gridData import Grid
g = Grid('test.dx')
g.origin
array([ 20.1,   3. , -10. ])

for i in g.centers():
    print(i)

[20.6  3.5 -9.5]
[20.6  3.5 -8.5]
[20.6  4.5 -9.5]
[20.6  4.5 -8.5]
[21.6  3.5 -9.5]
[21.6  3.5 -8.5]
[21.6  4.5 -9.5]
[21.6  4.5 -8.5]

[orbeckst]

See discussion #77 (comment) regarding APBS DX vs official DX. Please

• add a keyword argument to the centers() function "origin_centered=False" and add documentation to explain the choices. True implements your shift, False keeps the original behavior. Summarize the discussion Centers fucntion is not giving the right coordinates #77 (comment).
• add code to switch between True and False, perhaps easiest is something along

  offset = 0.5 * self.delta if not origin_centered else 0
  ...
      yield self.delta * numpy.array(idx) + self.origin + offset

• add a .. versionchanged:: entry to the docs for adding the new keyword
• update CHANGELOG
• add yourself to AUTHORS
• add tests for both cases (True and False)

Just ask in the comments if you have questions.

[orbeckst]

Note that the tests should pass.

[giacomofiorin]

@acruzpr @orbeckst Are you 100% sure that APBS uses the corners rather than the centers of the bins? The number of points typically used in APBS is a power of 2 plus 1:
https://apbs-pdb2pqr.readthedocs.io/en/latest/apbs/input/elec/dime.html#dime
which is consistent with the center of the left-most bin being a round number.

It is true that people may interpret the xyz coordinates of a volumetric map differently depending on what they are doing. But it would be prudent in my opinion to confirm first that there is an inconsistency between the various implementations of this format before adding the optional flag.

[orbeckst]

Fair enough. Have to look at APBS again. Thanks for your comments!!

…

Am 17.04.2020 um 18:57 schrieb Giacomo Fiorin ***@***.***>:  @acruzpr @orbeckst Are you 100% sure that APBS uses the corners rather than the centers of the bins? The number of points typically used in APBS is a power of 2 plus 1: https://apbs-pdb2pqr.readthedocs.io/en/latest/apbs/input/elec/dime.html#dime which is consistent with the center of the left-most bin being a round number. It is true that people may interpret the xyz coordinates of a volumetric map differently depending on what they are doing. But it would be prudent in my opinion to confirm first that there is an inconsistency between the various implementations of this format before adding the optional flag. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[acruzpr]

Could you please let me know if what I did is correct...

In the changelog, I added a new entry in current version 0.6

??/??/2020 eloyfelix, renehamburger1993, acruzpr

• 0.6.0

Enhancements

• Allow parsing/writing gzipped DX files
• Allow the generation of coordinates for the cell centers using the origin as defined by APBS DX file format or as defined by the Original DX file format (Fix Centers function #78)

Fixes

• fix initialization of mutable instance variable of Grid class (metadata dict) (core.Grid metadata badly initialized #71)
• fix multiple init calls (multiple Grid __init__ calls can break extended Grid classes #73)

For the documentation:
"""Returns the coordinates of the centers of all grid cells as an iterator.

    Parameters
    ----------
    origin_centered : bool
        When is set to True the origin will be set to the lower-left corner of the grid following
        the APBS DX format specifications.

        When is set to False the origin will be set to the center of the first grid cell following
        the official DX format specifications.

        .. versionchanged:: 0.6.0
         New *origin_centered* keyword argument
    """

This looks fine?

[orbeckst]

We first have to sort out what origin means to APBS, see discussion in #77 with APBS's original author.

@acruzpr do you have evidence that supports the interpretation that APBS considers the origin the corner of a cell instead of the center? Please provide it in the discussion on #77.

[orbeckst]

You made changes as necessary (I have minor comments, see inline), but I am putting this on hold until we know exactly what APBS does (see discussion on the issue #77).

[orbeckst]

Yes, that's how you add entries.

There's a space missing

Suggested change

	* Allow the generation of coordinates for the cell centers using the origin as defined by APBS DX file format
	or as defined by the Original DX file format (#78)

and we first have to make sure that we understand what APBS thinks, see #77

[orbeckst]

de-dent, so that it sits under everything

Suggested change

	.. versionchanged:: 0.6.0
	New *origin_centered* keyword argument
	.. versionchanged:: 0.6.0
	New *origin_centered* keyword argument

[RMeli]

@acruzpr @orbeckst is this PR still relevant?

[orbeckst]

Technically speaking, yes, it is, because I dropped the ball on the discussion #77 and never got to a decision — apologies to everyone.

I had a look at the discussion again but couldn't come to a conclusion after just reading it once. I'll try to make some time to look at it in more detail.