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

ENH: write denoising stats to artifact #96

Merged
merged 2 commits into from
Apr 25, 2018
Merged

Conversation

ebolyen
Copy link
Member

@ebolyen ebolyen commented Apr 19, 2018

fixes #79
This was more work than I expected.
Some things that might improve this in the future:

  • Metadata seems like an obvious "super format". Is there a good way to indicate what columns are meant to be present and auto-generate a format from that?
  • Framework could have transformers built in for Metadata and sub-formats
  • reading/writing TSV files is still a frustrating experience, and there's probably still a million holes. Can we use the above ideas to create a generic and robust parser?

TODO:

  • all the tests are borked.

@ebolyen ebolyen added the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label Apr 19, 2018
@ebolyen ebolyen added this to Backlog (Automated) in 2018.4 via automation Apr 19, 2018
@ebolyen ebolyen moved this from Backlog (Automated) to In Progress in 2018.4 Apr 19, 2018
@thermokarst thermokarst self-assigned this Apr 20, 2018
@thermokarst
Copy link
Contributor

I can take a stab at testing and getting the PR finalized. Probably later today (?) 🙏 .

@thermokarst thermokarst removed the stat:DO-NOT-MERGE Please do not merge this until this label has been removed. label Apr 25, 2018
@thermokarst thermokarst self-requested a review April 25, 2018 03:49
Copy link
Contributor

@thermokarst thermokarst left a comment

Choose a reason for hiding this comment

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

Proof tested hands-on (while adding in tests) - this looks great to me, @ebolyen! Thanks!

@thermokarst thermokarst assigned ebolyen and unassigned thermokarst Apr 25, 2018
@thermokarst
Copy link
Contributor

If my tests and other changes look okay to you @ebolyen, feel free to hit that "Squash and merge" button! ⏭

@thermokarst thermokarst moved this from In Progress to In Review in 2018.4 Apr 25, 2018
@ebolyen ebolyen merged commit 02b9ad6 into qiime2:master Apr 25, 2018
2018.4 automation moved this from In Review to Completed Apr 25, 2018
@ebolyen
Copy link
Member Author

ebolyen commented Apr 25, 2018

Thanks @thermokarst!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2018.4
  
Completed
Development

Successfully merging this pull request may close these issues.

Create report visualizer
2 participants