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 biom2 datatype #4519

Merged
merged 18 commits into from Sep 7, 2017

Conversation

Projects
None yet
6 participants
@shiltemann
Copy link
Member

commented Aug 30, 2017

Hi all,

this adds the biom2 (hdf5-formatted) metagenomics datatype (http://biom-format.org/documentation/biom_format.html)

also adds converters to and from the biom1 (json-formatted) datatype

shiltemann added some commits Aug 29, 2017

add h5py requirement
for extracting metadata from biom2/hdf5 datatypes

@galaxybot galaxybot added the triage label Aug 30, 2017

@galaxybot galaxybot added this to the 17.09 milestone Aug 30, 2017

shiltemann added some commits Aug 30, 2017

@bgruening bgruening added area/datatypes and removed triage labels Aug 30, 2017

@bgruening
Copy link
Member

left a comment

Thanks @shiltemann!


def set_peek(self, dataset, is_multi_byte=False):
if not dataset.dataset.purged:
dataset.peek = "Biom2 (HDF5) file"

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 30, 2017

Member

Maybe we can add here a little bit metadata as well.

This comment has been minimized.

Copy link
@shiltemann

shiltemann Aug 30, 2017

Author Member

good idea

@@ -32,6 +32,7 @@ six==1.10.0
Whoosh==2.7.4
testfixtures==4.10.0
galaxy_sequence_utils==1.0.2
h5py==2.7.0

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 30, 2017

Member

I think we need a wheel for this on cargo-port. A start might be here: galaxyproject/starforge#145

This comment has been minimized.

Copy link
@shiltemann

shiltemann Aug 30, 2017

Author Member

seems there was also already another PR for this too from a while back? galaxyproject/starforge#44 ..what's the best way forward?

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 7, 2017

Member

The wheels are built upstream and are available in PyPI so I don't think anything else should be needed here. I'll copy them to wheels.galaxyproject.org so we have them in case they disappear.

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 7, 2017

Member

Might as well update to 2.7.1 while you're in here.

elif 'format' in attributes: # biom 2.0
dataset.metadata.format = attributes['format']
dataset.metadata.type = attributes['type']
dataset.metadata.shape = str(list(attributes['shape']))

This comment has been minimized.

Copy link
@bgruening

bgruening Aug 30, 2017

Member

is this supposed to be a tuple? Like it is here the brackets will end up in the metadata as well, isn't it?

This comment has been minimized.

Copy link
@shiltemann

shiltemann Aug 30, 2017

Author Member

indeed, but my concern was that anything that's not a string was not being displayed in the GUI when looking at the metadata, and users might quite like to see that information. But sounds like that should proably be fixed elsewhere then (same for nnz, it is an int but doesn't show up in interface unless I cast it to string)

shiltemann added some commits Aug 30, 2017

@natefoo
Copy link
Member

left a comment

Thanks @shiltemann! Just one issue with the merge error in data.py that crept in from dev. I am not a datatype expert but this looks good to me.

import tempfile
import zipfile
from json import dumps
import h5py

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 7, 2017

Member

Technically for PEP-8 this should be:

import zipfile
from json import dumps

import h5py
import pysam
from bx.seq.twobit import TWOBIT_MAGIC_NUMBER, TWOBIT_MAGIC_NUMBER_SWAP, TWOBIT_MAGIC_SIZE
@@ -369,7 +369,6 @@ def display_data(self, trans, data, preview=False, filename=None, to_ext=None, *
if os.path.exists(file_path):
if os.path.isdir(file_path):
tmp_fh = tempfile.NamedTemporaryFile(delete=False)
tmp_file_name = tmp_fh.name

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 7, 2017

Member

This was due to a bad merge elsewhere in dev, it shouldn't be deleted. It'll fix the failing tests.

This comment has been minimized.

Copy link
@shiltemann

shiltemann Sep 7, 2017

Author Member

ah gotcha, reverted it, thanks :)

@@ -32,6 +32,7 @@ six==1.10.0
Whoosh==2.7.4
testfixtures==4.10.0
galaxy_sequence_utils==1.0.2
h5py==2.7.0

This comment has been minimized.

Copy link
@natefoo

natefoo Sep 7, 2017

Member

Might as well update to 2.7.1 while you're in here.

@natefoo natefoo referenced this pull request Sep 7, 2017

Closed

Archiving wheels #131

shiltemann added some commits Sep 7, 2017

Revert "remove unused variable"
This reverts commit 4119caf.
@natefoo

natefoo approved these changes Sep 7, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Sep 7, 2017

Looks good! Travis osx boxes look hung, but everything else is passing nicely.

@dannon dannon merged commit a8df872 into galaxyproject:dev Sep 7, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 290 tests run, 4 skipped, 0 failed.
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 45 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.