-
Notifications
You must be signed in to change notification settings - Fork 81
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
Generate alpha diversity graphs for each metadata category of interest #209
Conversation
Ready for review |
One test passed one failed due to timeout. Should still be good to review. |
… generate-alpha-diversity-per-sample
ping @EmbrietteH @ekopylova |
3 similar comments
ping @EmbrietteH @ekopylova |
ping @EmbrietteH @ekopylova |
ping @EmbrietteH @ekopylova |
filename = '-'.join([name, cleaned_col]) + '.txt' | ||
|
||
with open(join(out_dir, filename), 'w') as f: | ||
for otu, val in zip(ids, table.data(col)): | ||
if val == 0.0: | ||
continue | ||
f.write('%s\t%s\n' % (otu, str(val))) | ||
|
||
|
||
def cat_alpha_plots(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generates images, and the core function generating the image is tested, so I'm not sure how to unit test this. Do you have suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm how about computing the md5 (ex. self.assertFileMD5()
in this package)? Otherwise simply verifying your output file (shannon_path, pd_path) exists as you expect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hesitant to do MD5 because it could be sliiiightly rendered differently depending on the system tests are run on. Checking file is output would work though, and also makes sure the proper naming for files is there so the website can use them. Adding that.
lualatex seems to be a dependency but don't see it mentioned anywhere |
@squirrelo a few comments |
The lualatex is needed for rendering the barcode results PDFs. |
Test added for file names, but there is not a way to actually test the graph itself. |
02a524b
to
8b65db1
Compare
8b65db1
to
cf6cc45
Compare
Travis is not behaving for testing the rendering, and given that there are no other tests for rendering in the codebase, how much of a priority/how imperative is it that we have the render tests, given we're not checking the figure that is rendered and instead just the filename. |
Just checked the error really fast. It looks like the error is saying that the label "70+" is not in the axis. I'm unsure on how the figure is generated and I understand that systems can change the output figure a bit. However, I think that changing the axis is not a small change and it should be ensure that the axis are as we expect. It may be a bit more helpful to see an example of the image generated attached in here. |
That error is the latest in a long string of errors, mostly having to do with the latex not rendering correctly or at all. Only the latest non-working commit is in the PR for brevity's sake. |
Aaaand now tests are passing. Ok, cool. |
Code looks good - however, before merging, I'm gonna pulldown this code and test it using the latest AG round |
2108b73
to
1f24ca0
Compare
ping @josenavas mergable? |
No it's not:
I though you told me that you did not modify the plot alpha function? |
The only change is to prepend the alpha diversity type, which we discussed offline. Investigating now.
|
Fixed the issue. Was missing parenthesis. |
This generates the highlighted category alpha diversity graphs, allowing comparison on the interactive page.
Requires #208