Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] First try at TNG writing #18

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

[WIP] First try at TNG writing #18

wants to merge 9 commits into from

Conversation

jbarnoud
Copy link
Contributor

@jbarnoud jbarnoud commented Jul 5, 2017

Adds a write method to the TNGFile class. At that point, this method
successfully writes the positions and the box shape. It tries to write
the time, but fails. It does not try to write velocities and forces yet,
nor does it ties to write the topology.

The written file can be read by pytng. It is also read by gmx check,
but yields the following message:

TNG library: Cannot uncompress data block. /home/jon/src/gromacs-2016.3/src/external/tng_io/src/lib/tng_io.c:5298

This is my first time really writing cython, and I am not completely sure about what I am doing regarding the TNG library. Feel free to assume I am wring when reading the code.

  • Write positions
  • Write box
  • Write time
  • Write velocities
  • Write forces
  • Add doc
  • Add tests

Writing the topology will likely fall out of scope for this specific PR.

@jbarnoud jbarnoud changed the title First try at TNG writing [WIP ]First try at TNG writing Jul 5, 2017
@jbarnoud jbarnoud changed the title [WIP ]First try at TNG writing [WIP] First try at TNG writing Jul 5, 2017
@codecov-io
Copy link

codecov-io commented Jul 5, 2017

Codecov Report

Merging #18 into master will decrease coverage by 15.73%.
The diff coverage is 7.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #18       +/-   ##
===========================================
- Coverage   75.92%   60.18%   -15.74%     
===========================================
  Files           2        2               
  Lines         162      211       +49     
===========================================
+ Hits          123      127        +4     
- Misses         39       84       +45
Impacted Files Coverage Δ
pytng/pytng.pyx 59.61% <7.69%> (-15.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2864651...5607bc3. Read the comment docs.

pytng/pytng.pyx Outdated
@@ -264,6 +331,64 @@ cdef class TNGFile:
self.step += 1
return TNGFrame(xyz, time, self.step - 1, box)

def _init_first_frame_write(self, int64_t n_atoms,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like such implicit methods. Please rather make it explicit and fail in the write method if no header information was supplied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is going to disappear. See bellow.

pytng/pytng.pyx Outdated

ok = tng_util_pos_write_interval_set(self._traj, 1)
if_not_ok(ok, 'Could not set position write interval')
ok = tng_util_box_shape_write_interval_set(self._traj, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do these functions do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that the library has a mechanism in place to skip some frames. From what I understand, you can call the write methods for every frame, but only every N will actually be written. These methods tell the library to write all the frames.

I just tried without and it works (previous iterations on the code didn't). So I will remove these 4 lines and move the rest of the method in write.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, this is needed. It seems that the interval is not 1 per default. As a result, not setting it (or setting it to anything else than 1) leads to only part of the frames being written.

ok = tng_util_pos_write(self._traj, self.step, &xyz[0, 0])
else:
ok = tng_util_pos_with_time_write(self._traj, self.step,
time, &xyz[0, 0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why write the time twice? That seems strange. Maybe we need to ask in the gromacs devel list

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know. But I am also not sure it is the way to do. I am still trying to figure things out.

@jbarnoud
Copy link
Contributor Author

jbarnoud commented Jul 8, 2017

I fixed the writing of the time. The time per frame has to be set for the time to be written.

I also fixed the error I had when reading the box with gromacs. By default the library compresses the box using the gzip algorithm which seems to cause the problem; not compressing the box fixes it. The reading of the box seems to be broken, though. Pytng reads the first box for all the frames, while gromacs reads it right. I'll write an issue when I'll have a bit of time to reproduce the issue.

Here is the notebook I use to test my experiments on pytng: https://gist.github.com/jbarnoud/fd560a13162307f6f9e12df942ba0ca1.

Here is an other notebook I use to have just the minimum logic necessary: https://gist.github.com/jbarnoud/b8f750f116b76aba1b205babc001b569. I wrote this second notebook with the idea of asking a few questions on the gromacs dev mailing list. I am curious about the gzip issue for the box; and there may be a better way to write the time.

One constraint with the time is that I want to be able to write arbitrary time for each frame. This means that I have to use only one frame per frame set which results in larger files. Also, I constrain the number of atoms to be constant throughout the trajectory while there is no reason to do so regarding the format alone. There is a need to be able to set some flags before starting to write. I still need to think about the API.

@jbarnoud
Copy link
Contributor Author

jbarnoud commented Jul 8, 2017

I asked some clarifications on the gromacs mailing list. The thread is there: https://mailman-1.sys.kth.se/pipermail/gromacs.org_gmx-developers/2017-July/009741.html.

Adds a `write` method to the `TNGFile` class. At that point, this method
successfully writes the positions and the box shape. It tries to write
the time, but fails. It does not try to write velocities and forces yet,
nor does it ties to write the topology.

The written file can be read by pytng. It is also read by `gmx check`,
but yields the following message:

    TNG library: Cannot uncompress data block. /home/jon/src/gromacs-2016.3/src/external/tng_io/src/lib/tng_io.c:5298
@kain88-de
Copy link
Member

urgh I'm getting segfaults left and right when I currently try to write anything.

@kain88-de
Copy link
Member

Fixed my own segfault. But we get a segfault in the garbage collection when I try to copy a trajectory with pytng. This only happens when I copy the positions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants