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

Serialization #154

Merged
merged 10 commits into from
Jul 26, 2016
Merged

Serialization #154

merged 10 commits into from
Jul 26, 2016

Conversation

iskandr
Copy link
Contributor

@iskandr iskandr commented Jul 20, 2016

We previously had implementations of __getstate__ and __setstate__ which (1) didn't work for non-human genomes and (2) used fragile positional arguments. This PR:

  • Creates a Serializable base class for Genome, EnsemblRelease, Gene, Transcript, and Exon, Species. This allows us to share serialization/flattening code between these objects with copy/paste madness.
  • Adds to_json/from_json and to_dict/from_dict methods to all serializable objects.
  • Moves normalize_chromosome and normalize_strand to a separate module (no changes to actual code)
  • Moved global state from species.py onto the Species class

This change is Reviewable

@@ -35,12 +35,6 @@ def __str__(self):
return "Exon(exon_id=%s, gene_name=%s, contig=%s, start=%d, end=%s)" % (
self.id, self.gene_name, self.contig, self.start, self.end)

def __repr__(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened to repr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of our implementations just call str so I made that the default in Serializable. Maybe that object needs to be renamed to something like SaneDefaultsForClassesWhichImplementToDict.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+0.7%) to 80.679% when pulling dc38ec7 on json_serialization into 3108c6e on master.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+0.6%) to 80.607% when pulling 90bdfc1 on json_serialization into 3108c6e on master.


def __str__(self):
fields_str = ", ".join(
"%s=%s" % (k, v) for (k, v) in sorted(self.to_dict().items()))
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see a use-case for not wanting to rely on sorting, but rather on some instrinsic ordering (e.g. ID first, name second, etc.). Any interest in using OrderedDicts instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean just use the order of items() but have to_dict() return an OrderedDict?

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+0.6%) to 80.607% when pulling 90bdfc1 on json_serialization into 3108c6e on master.


def _object_to_primitive_types(self):
"""
Given an instance of a Python object, returns a tuple whose
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting confused between the tuple here and the one above. Maybe rename _class_to_tuple to _class_to_full_name_tuple and _tuple_to_class to _full_name_tuple_to_class?

@tavinathanson
Copy link
Contributor

LGTM % comments 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 81.356% when pulling 3649597 on json_serialization into 3108c6e on master.

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+1.3%) to 81.356% when pulling 3649597 on json_serialization into 3108c6e on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 80.544% when pulling 8d7bf72 on json_serialization into 3108c6e on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+0.5%) to 80.568% when pulling 8d7bf72 on json_serialization into 3108c6e on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+1.1%) to 81.503% when pulling b341b14 on json_serialization into f2e4eea on master.

@coveralls
Copy link

coveralls commented Jul 22, 2016

Coverage Status

Coverage increased (+1.1%) to 81.503% when pulling b341b14 on json_serialization into f2e4eea on master.

@coveralls
Copy link

coveralls commented Jul 26, 2016

Coverage Status

Coverage increased (+1.06%) to 81.419% when pulling 421498d on json_serialization into f2e4eea on master.

@iskandr iskandr merged commit 44b60df into master Jul 26, 2016
@iskandr iskandr deleted the json_serialization branch July 26, 2016 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants