Work around more DX parsing issues by other apps #58
Conversation
This change works around another example of poorly hard-coded DX parser (see also issue MDAnalysis#35). See the code below from the GridForcesGrid.C file in NAMD: fscanf(poten_fp, "object %*d class array type double rank 0 " "items %*d data follows\n"); which will only work if the quote character (introduced in 115b05a to appease the PyMol parser) is omitted.
Again, this is a workaround for an issue in the PyMol DX parser that fails to
parse tiny numbers (i.e. with an exponent lower than -100). If it seems
inconsistent with requiring "type double", it is because the DX parser uses a
"float" to read the data:
static int read_dx_data(void *v, int set, float *datablock,
float *colorblock) {
...
float grid;
...
Observed with PyMol up to 2.3.0.
giacomofiorin
force-pushed
the
dx-writer-fixes
branch
from
02a0473 to
6154342
Compare
|
The amended commit now handles also test cases where grids of integer values are being used. Note that the AppVeyor test fails due to Visual Studio configuration issues (probably unrelated to this PR). |
|
Hello, a brief reminder that the Travis test passes, whereas the AppVeyor test fails due to causes unrelated to this PR: |
|
@giacomofiorin sorry for the long silence – I haven't had a look at GridDataFormats in a while. In general, ping me with @orbeckst . I'll have a look at your contribution, thank you! |
There was a problem hiding this comment.
Minor comments and please
- add yourself to AUTHORS
- add an entry to CHANGELOG (under 0.5.0)
(I will do a quick release of 0.4.1 and a 0.5.0 section will appear soon, you just have to merge master into your branch.)
If you can think of a test that explicitly tests your functionality then add one, please.
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
=========================================
+ Coverage 86.42% 86.53% +0.1%
=========================================
Files 5 5
Lines 759 765 +6
Branches 109 110 +1
=========================================
+ Hits 656 662 +6
Misses 61 61
Partials 42 42
Continue to review full report at Codecov.
|
|
@orbeckst Thanks. Both new features are used within |
|
Ok, then add yourself to AUTHORS and add an entry to CHANGELOG. I don't have an updated CHANGELOG yet so use the current one and just add your entry under 0.4.1 but I will put whatever you write in the correct spot before I merge. |
|
I did that in the merge commit, except I put 0.5.0 as suggested. Feel free to edit the description as needed. |
gridData/core.py
Outdated
| file_format : {'dx', 'pickle', None} (optional) | ||
| output file format, the default is "dx" | ||
|
|
||
| type : str (optional) | ||
| for DX, set the output DX array type, e.g., "double" or | ||
| "float"; note that PyMOL only understands "double" (see |
There was a problem hiding this comment.
In the light of your comment
The most recent versions of PyMol now support type float: schrodinger/pymol-open-source@4bbe425
which is a reasonable decision, given that it is actually not parsing doubles correctly :-)
is this comment in the docs still relevant?
There was a problem hiding this comment.
It may still be relevant if the numbers in the DX map are outside the allowable range for a 32-bit float, regardless of which type is stated in the DX header. You may want to actually discourage using type=double because Pymol doesn't really support it.
However, this is really a Pymol issue, and in all practical scenarios it is avoided now with the new format. So I've just removed the comment for simplicity.
|
@giacomofiorin thank you for your contribution. Sorry it took such a long time. Next time ping me with '@orbeckst' to get my attention. |
- fixed doc building failure (only encountered locally and on RTD, not in tests) - included @giacomofiorin in the author list displayed in the docs (was forgotten in PR #58)
The code below from the GridForcesGrid.C file in NAMD:
will only work if the quote character (introduced in 115b05a to work around a similar problem in the PyMol parser - see #35) is omitted. This is done via the optional
typequotekeyword, to accompanytype.The other commit has to do with another inconsistency in PyMol. Its DX parser fails to parse tiny numbers (i.e. with an exponent lower than -100) despite using a
floatfor parsing and the%fformat in the function:The most recent versions of PyMol now support
type float:schrodinger/pymol-open-source@4bbe425
which is a reasonable decision, given that it is actually not parsing
doubles correctly :-)