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

Add plain msgpack serialization #267

Merged
merged 5 commits into from
Jun 30, 2021
Merged

Add plain msgpack serialization #267

merged 5 commits into from
Jun 30, 2021

Conversation

bennybp
Copy link
Contributor

@bennybp bennybp commented Jun 29, 2021

Description

Adds ability to serialize to plain msgpack format.

The current msgpack-ext encoding serializes numpy arrays as numpy arrays, which is not portable (that is, deserializing would require python and qcelemental). This adds a plain msgpack encoding that converts numpy arrays to lists, as is done with the existing json encoding.

Changelog description

Adds plain msgpack serialization

Status

  • Code base linted
  • Ready to go

@bennybp bennybp added the Enhancement Project enhancement discussion label Jun 29, 2021
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #267 (ab95f19) into master (5e413e6) will increase coverage by 0.00%.
The diff coverage is 93.33%.

Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

qcelemental/util/serialization.py Show resolved Hide resolved
@bennybp
Copy link
Contributor Author

bennybp commented Jun 29, 2021

Added to the init file and fixed the test. I guess I didn't need to fix the test, since dtype is inferred by the extension, but it's nice to be explicit I guess.

For the to/from file, I removed the msgpack-ext to make it more consistent. So json-ext and msgpack-ext aren't valid for files.

Copy link
Collaborator

@loriab loriab left a comment

Choose a reason for hiding this comment

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

how're these for a sol'n? I'm trying to avoid changing current behavior.

if you date the v0.21 release in changelog, I won't have to make an extra PR

qcelemental/models/molecule.py Outdated Show resolved Hide resolved
qcelemental/models/molecule.py Outdated Show resolved Hide resolved
bennybp and others added 2 commits June 29, 2021 13:52
elif dtype == "msgpack-ext":
with open(filename, "rb") as infile_bytes:
data = deserialize(infile_bytes.read(), encoding="msgpack-ext")
dtype = "dict"
Copy link
Collaborator

Choose a reason for hiding this comment

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

since for reading json=json-ext and msgpack=msgpack-ext, aren't only 2 if's needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, although we should make it explicit. Ie, for json, use json-ext and for msgpack, use msgpack-ext (with comment that the -ext are backwards compatible)

@loriab loriab merged commit 6e101fb into MolSSI:master Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Project enhancement discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants