Accounting for different summary intervals in summarize-paired?

[fedarko]

It looks like the code for _summarize() assumes that both stats inputs have the same summary interval:

https://github.com/biocore/songbird/blob/2c4cf016ba8689b48fff6ee7eb5231068da76b55/songbird/q2/_summary.py#L57-L60

As far as I'm aware, this isn't documented in the user-facing API (I didn't know this was the case until today, and I'm guessing not a lot of other Songbird users know this). Is this still a restriction? I tested this by running the same qiime songbird multinomial command twice (once with a summary interval of 5, and once with a summary interval of 1)—attaching a picture showing the resulting summarize-paired visualization (baseline = interval of 1, model = interval of 5).

This looks fine. The curves' starting x-axis positions are different, but that seems to be a natural result of when the recording started (so not really a problem)—in the summarize-single for the model curve, it also starts at around 10k iterations.

tldr

If having different summary intervals isn't a problem, I can close this issue and remove the corresponding note in the code. If it is a problem, here are some ideas for improving the user experience that I'd be happy to add:

1. Specify this restriction clearly in the summarize-paired documentation
2. Throw an error in _convergence_plot under the if baseline is not None: block if len(regression.index) != len(baseline.index)

Let me know what you think. Thanks!

[mortonjt]

Hi @fedarko , I'm not sure why I didn't answer this issue - sorry about not responding sooner.

Note that the reason why we had this interface in the first place was because qiime2 is currently incompatible with tensorboard - it is not clear how to register tensorboard as a qiime2 visualizer.

In short - yes, having unaligned intervals is a problem. But this has already been resolved in the standalone version in tensorboard. I hazard against further development and redevelop another tensorboard because of incompatibilities with qiime2. So I'm going to close this and mark a wont-fix tag for the time being.

Note that if we can find a way to load tensorboard into q2view - that would be the ultimate solution, but its not clear how feasible this is.

[fedarko]

@mortonjt -- likewise, sorry for the delay on my end :)

In short - yes, having unaligned intervals is a problem. But this has already been resolved in the standalone version in tensorboard. I hazard against further development and redevelop another tensorboard because of incompatibilities with qiime2. So I'm going to close this and mark a wont-fix tag for the time being.

So, I completely understand your hesitance to spend time maintaining this interface when there are alternative solutions that other groups with more inertia have already been maintaining. However, if the current solutions through Songbird are straight-up incorrect, then (I believe) we should at least make note of this rather than lead others down incorrect paths. Creating another Tensorboard is a ton of work; but at this point the q2 summary interface is already "good enough", so the main problems that this issue (and #104?) are trying to fix are just resolving problems with the extant interface rather than adding in more functionality (I agree with you that people should just use the standalone version + Tensorboard if they need finer grained control).

In the case of this specific issue: do different summary intervals do anything besides make the paired summary plots look kind of ugly? If the different intervals actually cause the interpretation to be wrong, then IMO that is a problem that should be addressed.

"Addressed" in this case doesn't mean making you more code, necessarily -- one easy compromise solution (that I'd be happy to submit a PR for sometime) would just be adding some HTML to the paired summary output that says "hey if things look funky you should make sure your summary results were generated using the same intervals", or something like that. This would at least make users aware of this problem, if it really is a problem.

[mortonjt]

Very ok with adding in extra checks / error messages and / or Readme updates

On Wed, Jan 15, 2020, 10:31 PM Marcus Fedarko [email protected] wrote:

@mortonjt https://github.com/mortonjt -- likewise, sorry for the delay on my end :)

In short - yes, having unaligned intervals is a problem. But this has already been resolved in the standalone version in tensorboard. I hazard against further development and redevelop another tensorboard because of incompatibilities with qiime2. So I'm going to close this and mark a wont-fix tag for the time being.

So, I completely understand your hesitance to spend time maintaining this interface when there are alternative solutions that other groups with more inertia have already been maintaining. However, if the current solutions through Songbird are straight-up incorrect, then (I believe) we should at least make note of this rather than lead others down incorrect paths. Creating another Tensorboard is a ton of work; but at this point the q2 summary interface is already "good enough", so the main problems that this issue (and #104 https://github.com/biocore/songbird/issues/104?) are trying to fix are just resolving problems with the extant interface rather than adding in more functionality (I agree with you that people should just use the standalone version + Tensorboard if they need finer grained control).

In the case of this specific issue: do different summary intervals do anything besides make the paired summary plots look kind of ugly? If the different intervals actually cause the interpretation to be wrong, then IMO that is a problem that should be addressed.

"Addressed" in this case doesn't mean making you more code, necessarily -- one easy compromise solution (that I'd be happy to submit a PR for sometime) would just be adding some HTML to the paired summary output that says "hey if things look funky you should make sure your summary results were generated using the same intervals", or something like that. This would at least make users aware of this problem, if it really is a problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/biocore/songbird/issues/75?email_source=notifications&email_token=AA75VXPFBLZW7LN3KKDSS2LQ57IJBA5CNFSM4IYEU2FKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJCUQYI#issuecomment-574965857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA75VXIU25XUTIENE4XXBTDQ57IJBANCNFSM4IYEU2FA .

[fedarko]

Sweet, I'll see if I can find time to send in a PR this weekend. Would you mind re-opening this issue just for reference?