ENH: Q2 summary interval allow float #108
Conversation
There was a problem hiding this comment.
@gwarmstrong Looks good! There are a few things I think could be made better (mostly minor stuff like code documentation), but by and large this should be good to go once those are addressed.
| md.name = 'sampleid' | ||
| md = qiime2.Metadata(md) | ||
|
|
||
| exp_beta = clr(clr_inv(np.hstack((np.zeros((2, 1)), self.beta.T)))) |
There was a problem hiding this comment.
It looks like this line (as well as a few others towards the end of this function, e.g. the npt.assert_allclose lines) were taken from test_fit() in this file. That's not a problem, but this should be commented to make that clear if possible -- the clr(clr_inv()) stuff in particular is apparently kind of a hack, and we should try to document where it's used so it could ideally be taken care of in the future.
| res_biplot = q2_res_biplot.view(OrdinationResults) | ||
| except Exception: | ||
| raise AssertionError('res_biplot unable to be coerced to ' | ||
| 'OrdinationResults') |
There was a problem hiding this comment.
I was gonna ask you to use a more specific error type than just Exception but I tried it out and, yep, you get a basic Exception when transformations fail. The more you know!
(Maaybe worth adding a comment mentioning this just so that future people reviewing this code don't have the same reaction I just did...)
Adds the ability to use a float for
summary-intervalin Q2.Would be nice to get more granular feedback for models that run very quickly, such as below:
Where the model converge quickly (just a couple seconds) so there are only a very few number of updates before convergence.