WIP: TopologyAttr registration; prototype Topology json serialization #831

Closed
wants to merge 1 commit into
from

Conversation

Projects
5 participants
@dotsdl
Member

dotsdl commented Apr 21, 2016

We now register TopologyAttrs using the same metaclassing mechanism as
used for Parsers, Readers, and Writers. This is necessary in order to
deserialize Topology objects that have been serialized to JSON.

An existing Topology can now be serialized to JSON with:

top.to_json('topology.json')

and a Topology can be generated from its JSON form with:

top = Topology.from_json('topology.json')

Individual TopologyAttrs must define how to serialize themselves and
to deserialize their data to make an instance of themselves for this to
work.

Addresses #643.

TopologyAttr registration; prototype Topology json serialization
We now register TopologyAttrs using the same metaclassing mechanism as
used for Parsers, Readers, and Writers. This is necessary in order to
deserialize Topology objects that have been serialized to JSON.

An existing Topology can now be serialized to JSON with:

```python

top.to_json('topology.json')

```

and a Topology can be generated from its JSON form with:

```python

top = Topology.from_json('topology.json')

```

Individual TopologyAttrs must define how to serialize themselves and
to deserialize their data to make an instance of themselves for this to
work.

Addresses #643.
@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Apr 21, 2016

Member

Looks good. Is there a reason why we can't use something faster like Feather?

Member

richardjgowers commented Apr 21, 2016

Looks good. Is there a reason why we can't use something faster like Feather?

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Apr 21, 2016

Member

Ok feather is pandas based, but wouldn't a binary format allow us to a) save space and b) enable faster loading?

Member

richardjgowers commented Apr 21, 2016

Ok feather is pandas based, but wouldn't a binary format allow us to a) save space and b) enable faster loading?

@dotsdl

This comment has been minimized.

Show comment
Hide comment
@dotsdl

dotsdl Apr 21, 2016

Member

@richardjgowers a binary format could definitely give better performance (both space and speed), but at the cost of perhaps a heavyweight dependency and a less transparent storage scheme. JSON is 1) universal, and 2) simple.

I'm happy to work toward an effort using a decent binary format, but I think JSON isn't bad. In fact, getting back the topology for a system with ~10M atoms could be done on my laptop within
10 seconds. It's writing that takes a good deal longer. In both cases, though, we might expect speedups with a good binary format, e.g. HDF5.

Member

dotsdl commented Apr 21, 2016

@richardjgowers a binary format could definitely give better performance (both space and speed), but at the cost of perhaps a heavyweight dependency and a less transparent storage scheme. JSON is 1) universal, and 2) simple.

I'm happy to work toward an effort using a decent binary format, but I think JSON isn't bad. In fact, getting back the topology for a system with ~10M atoms could be done on my laptop within
10 seconds. It's writing that takes a good deal longer. In both cases, though, we might expect speedups with a good binary format, e.g. HDF5.

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Apr 22, 2016

Member

tests??

Member

orbeckst commented Apr 22, 2016

tests??

+ for topattr in self.attrs:
+
+ # these don't hold any data of their own
+ if isinstance(topattr, (Atomindices, Resindices, Segindices)):

This comment has been minimized.

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

What about having {Atom,Res,Seg}indices._to_json raise an AttributeError? Either by not having them defined or by having them explicitely raise the exception? Then, these attributes will be ignored thanks to the try bellow. This would save us this test, avoid special cases, and avoid a continue that can be error prone if this loop get extended in the future.

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

What about having {Atom,Res,Seg}indices._to_json raise an AttributeError? Either by not having them defined or by having them explicitely raise the exception? Then, these attributes will be ignored thanks to the try bellow. This would save us this test, avoid special cases, and avoid a continue that can be error prone if this loop get extended in the future.

This comment has been minimized.

@richardjgowers

richardjgowers Apr 28, 2016

Member

This would work, but we should probably add a warning when an attribute isn't being serialised, which these guys wouldn't want.

@richardjgowers

richardjgowers Apr 28, 2016

Member

This would work, but we should probably add a warning when an attribute isn't being serialised, which these guys wouldn't want.

+ pass
+
+ # write out
+ with open(filename, 'w') as f:

This comment has been minimized.

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

Could it be util.anyopen? This would allow to compress the json topology, but also to do funny things with StreamIO to interface this with other tools (in the same way as the interface with nglview writes a PDB file in a stream).

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

Could it be util.anyopen? This would allow to compress the json topology, but also to do funny things with StreamIO to interface this with other tools (in the same way as the interface with nglview writes a PDB file in a stream).

+ """
+ from . import _TOPOLOGYATTRS
+
+ with open(filename, 'r') as f:

This comment has been minimized.

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

anyopen?

@jbarnoud

jbarnoud Apr 25, 2016

Contributor

anyopen?

@dotsdl dotsdl changed the title from TopologyAttr registration; prototype Topology json serialization to WIP: TopologyAttr registration; prototype Topology json serialization May 12, 2016

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de May 15, 2016

Member

@dotsdl have you made progress on this?

Member

kain88-de commented May 15, 2016

@dotsdl have you made progress on this?

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers May 15, 2016

Member

this isn't really time critical, it's an addon to 363

Member

richardjgowers commented May 15, 2016

this isn't really time critical, it's an addon to 363

@dotsdl

This comment has been minimized.

Show comment
Hide comment
@dotsdl

dotsdl May 16, 2016

Member

@kain88-de haven't touched it, but will focus on suggestions made here soon. It's mainly meant as a proof-of-concept for a benefit we'd been wanting to leverage the new Topology object for.

Member

dotsdl commented May 16, 2016

@kain88-de haven't touched it, but will focus on suggestions made here soon. It's mainly meant as a proof-of-concept for a benefit we'd been wanting to leverage the new Topology object for.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Nov 7, 2016

Member

This will have to be re targeted against develop now.

Member

kain88-de commented Nov 7, 2016

This will have to be re targeted against develop now.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Nov 27, 2016

Member

I'll close this now as it has to be reopened anyway.

Member

kain88-de commented Nov 27, 2016

I'll close this now as it has to be reopened anyway.

@kain88-de kain88-de closed this Nov 27, 2016

@kain88-de kain88-de deleted the issue-363-json branch Jan 24, 2018

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