Fix Centers function #78

acruzpr · 2020-04-17T08:44:30Z

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]

update

Centers should be displaced by self.delta * 0.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 · 2020-04-18T01:40:19Z

Note that the tests should pass.

giacomofiorin · 2020-04-18T01:57:17Z

@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 · 2020-04-18T02:43:40Z

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 · 2020-04-23T18:16:03Z

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?

…tests

orbeckst · 2020-05-18T16:59:29Z

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 · 2020-05-18T17:03:37Z

CHANGELOG


  * 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


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 · 2020-05-19T00:09:08Z

gridData/core.py

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


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 · 2023-08-12T15:39:20Z

@acruzpr @orbeckst is this PR still relevant?

orbeckst · 2023-08-12T17:41:41Z

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.

acruzpr added 2 commits April 17, 2020 04:21

Merge pull request #1 from MDAnalysis/master

e2b6702

update

Fix centers function

eb5356c

Centers should be displaced by self.delta * 0.5

orbeckst mentioned this pull request Apr 18, 2020

Centers fucntion is not giving the right coordinates #77

Open

orbeckst requested changes Apr 18, 2020

View reviewed changes

Added origin_centered keyword to the center function and updated the …

69f5191

…tests

orbeckst requested changes May 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Centers function #78

Fix Centers function #78

acruzpr commented Apr 17, 2020 •

edited by orbeckst

Loading

orbeckst left a comment

orbeckst commented Apr 18, 2020

giacomofiorin commented Apr 18, 2020

orbeckst commented Apr 18, 2020 via email

acruzpr commented Apr 23, 2020

orbeckst commented May 18, 2020

orbeckst left a comment

orbeckst May 18, 2020

orbeckst May 19, 2020

RMeli commented Aug 12, 2023

orbeckst commented Aug 12, 2023

	* 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)

		.. versionchanged:: 0.6.0
		New origin_centered keyword argument

Fix Centers function #78

Are you sure you want to change the base?

Fix Centers function #78

Conversation

acruzpr commented Apr 17, 2020 • edited by orbeckst Loading

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Apr 18, 2020

giacomofiorin commented Apr 18, 2020

orbeckst commented Apr 18, 2020 via email

acruzpr commented Apr 23, 2020

orbeckst commented May 18, 2020

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst May 18, 2020

Choose a reason for hiding this comment

orbeckst May 19, 2020

Choose a reason for hiding this comment

RMeli commented Aug 12, 2023

orbeckst commented Aug 12, 2023

acruzpr commented Apr 17, 2020 •

edited by orbeckst

Loading