biocore / biom-format

The Biological Observation Matrix (BIOM) Format Project
http://biom-format.org
Other
90 stars 95 forks source link

Create method to add more data (observation and samples) #475

Closed ElDeveloper closed 7 years ago

wasade commented 10 years ago

perhaps Table.append and Table.extend with axis args?

ElDeveloper commented 10 years ago

Yeah that sounds like a good idea, follows the design of the rest of the API. BTW @adamrp @teravest and I were wondering why was this not a method before? Is there a reason for this?

On (Jun-15-14|20:48), Daniel McDonald wrote:

perhaps Table.append and Table.extend with axis args?


Reply to this email directly or view it on GitHub: https://github.com/biocore/biom-format/issues/475#issuecomment-46139042

wasade commented 10 years ago

Because there was merge On Jun 16, 2014 7:59 AM, "Yoshiki Vázquez Baeza" notifications@github.com wrote:

Yeah that sounds like a good idea, follows the design of the rest of the API. BTW @adamrp @teravest and I were wondering why was this not a method before? Is there a reason for this?

On (Jun-15-14|20:48), Daniel McDonald wrote:

perhaps Table.append and Table.extend with axis args?


Reply to this email directly or view it on GitHub: https://github.com/biocore/biom-format/issues/475#issuecomment-46139042

— Reply to this email directly or view it on GitHub https://github.com/biocore/biom-format/issues/475#issuecomment-46189331.

wasade commented 10 years ago

why not use a merge? I think a merge actually is the best thing to do here

adamrp commented 10 years ago

Yeah, that was my suggestion, too. On Jun 24, 2014 11:53 AM, "Daniel McDonald" notifications@github.com wrote:

why not use a merge?

— Reply to this email directly or view it on GitHub https://github.com/biocore/biom-format/issues/475#issuecomment-47005640.

ElDeveloper commented 10 years ago

It makes sense but from the UI perspective add would be more intuitive (and I can see add using merge under the hood without a problem).

Yoshiki Vázquez-Baeza

On Jun 24, 2014, at 11:53 AM, Daniel McDonald notifications@github.com wrote:

why not use a merge?

— Reply to this email directly or view it on GitHub.

wasade commented 10 years ago

What about:

orthogonal

def add(self, v, id_, md, orthogonal_ids, axis):
    if axis == 'sample':
        v = v[:, newaxis]
        to_merge = Table(v, orthogonal_ids, [id_], None, md)
    else:
        to_merge = Table(v, [id_], orthogonal_ids, md, None)
     return self.merge(to_merge)
adamrp commented 10 years ago

I still don't think this is worth the API change -- what is the use case? The use case that I heard about was for developers to create examples, but IMHO that's not a great reason to add it to the API, when we could almost as easily use a merge in that niche case.

On Thu, Jun 26, 2014 at 8:21 AM, Daniel McDonald notifications@github.com wrote:

https://camo.githubusercontent.com/708383c85740ce4d9e691d1fb7e918b14c93952f/687474703a2f2f322e62702e626c6f6773706f742e636f6d2f2d3577654b55335f394a62672f55764c55424147706530492f4141414141414141416a6f2f7073636b76755f67374d302f73313630302f636173746c652b7370656563686c6573732e676966

What about:

orthogonal

def add(self, v, id_, md, orthogonal_ids, axis): if axis == 'sample': v = v[:, newaxis] to_merge = Table(v, orthogonalids, [id], None, md) else: tomerge = Table(v, [id], orthogonal_ids, md, None) return self.merge(to_merge)

— Reply to this email directly or view it on GitHub https://github.com/biocore/biom-format/issues/475#issuecomment-47239639.

wasade commented 10 years ago

seems reasonable

On Thu, Jun 26, 2014 at 1:23 PM, adamrp notifications@github.com wrote:

I still don't think this is worth the API change -- what is the use case? The use case that I heard about was for developers to create examples, but IMHO that's not a great reason to add it to the API, when we could almost as easily use a merge in that niche case.

On Thu, Jun 26, 2014 at 8:21 AM, Daniel McDonald notifications@github.com

wrote:

< https://camo.githubusercontent.com/708383c85740ce4d9e691d1fb7e918b14c93952f/687474703a2f2f322e62702e626c6f6773706f742e636f6d2f2d3577654b55335f394a62672f55764c55424147706530492f4141414141414141416a6f2f7073636b76755f67374d302f73313630302f636173746c652b7370656563686c6573732e676966>

What about:

orthogonal

def add(self, v, id_, md, orthogonal_ids, axis): if axis == 'sample': v = v[:, newaxis] to_merge = Table(v, orthogonalids, [id], None, md) else: tomerge = Table(v, [id], orthogonal_ids, md, None) return self.merge(to_merge)

— Reply to this email directly or view it on GitHub https://github.com/biocore/biom-format/issues/475#issuecomment-47239639.

— Reply to this email directly or view it on GitHub https://github.com/biocore/biom-format/issues/475#issuecomment-47268286.

wasade commented 10 years ago

@ElDeveloper, okay to close?

ElDeveloper commented 10 years ago

No, I think we should have add as a method, however I don't think this should be there for 2.1.

wasade commented 10 years ago

Sounds good, setting to 2.2 and assigning you for now

ElDeveloper commented 10 years ago

Sounds good.

On (Jul-01-14|13:57), Daniel McDonald wrote:

Sounds good, setting to 2.2 and assigning you for now


Reply to this email directly or view it on GitHub: https://github.com/biocore/biom-format/issues/475#issuecomment-47708966

ElDeveloper commented 10 years ago

@wdwvt1 this is the issue that I mentioned during your code review (when you asked what was the easiest way to add data to a table) last week.

wdwvt1 commented 10 years ago

Here was the use case (using merge now) I demonstrated in code review last week:

# remove features which don't meet criteria
fbt = bt.filter(_f, axis = 'observation', inplace = False)
# calculate the lost feature mass
lost_feature_mass = 1. - fbt.sum(axis = 'sample')
# create a new table with the lost feature mass as feature 'Other'
t = table.Table(lost_feature_mass, [lfm_name], fbt.ids(axis = 'sample'))
# merge tables and have lfm feature first to make plotting easier
return t.merge(fbt, sample = 'intersection', observation = 'union')

The merge accomplishes my objective just fine, but it seems silly to create a new table just for the purpose of adding a single feature.

ElDeveloper commented 10 years ago

The merge accomplishes my objective just fine, but it seems silly to create a new table just for the purpose of adding a single feature.

:+1:, I agree.

On (Jul-21-14|16:23), Will Van Treuren wrote:

Here was the use case (using merge now) I demonstrated in code review last week:

# remove features which don't meet criteria
fbt = bt.filter(_f, axis = 'observation', inplace = False)
# calculate the lost feature mass
lost_feature_mass = 1. - fbt.sum(axis = 'sample')
# create a new table with the lost feature mass as feature 'Other'
t = table.Table(lost_feature_mass, [lfm_name], fbt.ids(axis = 'sample'))
# merge tables and have lfm feature first to make plotting easier
return t.merge(fbt, sample = 'intersection', observation = 'union')

The merge accomplishes my objective just fine, but it seems silly to create a new table just for the purpose of adding a single feature.


Reply to this email directly or view it on GitHub: https://github.com/biocore/biom-format/issues/475#issuecomment-49679614

wasade commented 10 years ago

True. The Table provides the contextual information such as which element corresponds to what ID though. It would be a little ugly for an add method:

def add(self, id_, data, element_ids, axis='sample', metadata=None):
    ...

Where the element_ids need to be in index order with data, and be the corresponding IDs in the relevant axis of the Table. Do able, but still awkward. Would that be cleaner than the filter/merge stuff above?

wasade commented 7 years ago

@ElDeveloper, I think that #720 resolves this? Please reopen if necessary

ElDeveloper commented 7 years ago

👍!!

ElDeveloper commented 7 years ago

Though, just noticed the PR hasn't been merged 🤔

wasade commented 7 years ago

meh. Want to resolve that problem??? :)