Improved documentation and Q2 summary visualizations; centralizing parameters #80
Conversation
Relates to ideas brought up in biocore#72.
This should make it clearer that you need to actually use this for validating that your model is fitting properly. Even though this is an important step, I've seen many people download Songbird, run it once, and just ignore the tensorboard stuff because it doesn't look important. (I was also guilty of this for a while, so something something glass houses.) Making this more visible will help indicate to people how to use this tool properly.
As proposed in biocore#72.
Another item from biocore#72.
shouldn't cause the plots to get shifted to the right any more.
also highlights the label ("Pseudo Q-squared:") in <strong> tags.
should hopefully be a lot more usable now, but we'll see if i still feel that way tomorrow morning
my fork's README took into account lisa's change, so no need to do anything fancy when resolving the merge
just in the command-line examples! this makes the indentation levels consistent. ...as it should be ;)
also added a "what will this cover" section
... i think many computers would complain if you tried to name a directory <results>
Will incorporate this into the standalone script in a sec
I also ran the standalone script through black, since I didn't want to have to reformat the entire dang thing by hand. Can run the other files through black (or add this to the Makefile) if desired.
Closes biocore#77. This should make the "default" run of Songbird produce legible-ish diagnostic plots!
The fact that "n" is now optional in _summarize() means that there's ostensibly a weird case where _summarize() could be called *with* baseline stats but without n. So I made _summarize() throw an error in that case (should never happen but might as well be defensive), and also added a corresponding test case. This closes biocore#76.
getting close! biocore#72
also updated the README summary screenshot accordingly. I think now is kosher to submit a pr for biocore#72!
the interpreting graphs stuff pertained to the old tensorboard graph; updated to reference redsea graphs
|
Also, it'd probably be a good idea for multiple people to review this since this is a lot of changes, and it's important that they're correct. @lisa55asil if you'll be around tomorrow, would you mind sitting down and going over this with me? A lot of the documentation changes here involved your FAQs, so you'd probably be the most qualified person to comment on that :) |
Thanks @lisa55asil for the feedback!
This opts to specify things in less detail, in favor of being *correct*. Should close biocore#79.
|
Ok, Lisa's changes have been integrated in. We've both made some changes (I fixed the @mortonjt, pending your final approval I think this should be good to merge in. |
|
...made a few last-minute commits showing off using |
|
That's awesome! If its easy and you already have it a link to the output of
tabulating the differentials.qza from the Red Sea data would be helpful I
think.
…On Mon, Sep 23, 2019 at 3:31 PM Marcus Fedarko ***@***.***> wrote:
...made a few last-minute commits showing off using qiime metadata
tabulate on a differentials file, but i think that's it from me for right
now :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80?email_source=notifications&email_token=AE6EV4WU2TFHC6UYXCRBDWDQLE7SLA5CNFSM4IYSYWZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7MO2TA#issuecomment-534310220>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE6EV4UX7Y5V7ROPYBL4BI3QLE7SLANCNFSM4IYSYWZA>
.
|
based on feedback from @lisa55asil
|
Ok! I'm not sure adding a link to a QZV is the best idea, since I guess that'd have to be periodically updated if we also update Songbird... but I added a screenshot of the QZV output. I think that's a good advertisement for that functionality :) (here's a link to the screenshot) |
|
Noting |
|
@fedarko are you finished with edits? |
|
Going to make one more pass over everything today. I'll try to have it done
by 5pm EST.
Thanks!
Marcus
…On Tue, Sep 24, 2019, 9:00 AM Jamie Morton ***@***.***> wrote:
@fedarko <https://github.com/fedarko> are you finished with edits?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80?email_source=notifications&email_token=AA736PYLQZ5MMMHB364XZY3QLI2TPA5CNFSM4IYSYWZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7O4EAA#issuecomment-534626816>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA736P4Y5QAKAM273AMFUITQLI2TPANCNFSM4IYSYWZA>
.
|
Since a lot of people (me included) misunderstand this.
|
@mortonjt I'm done! Added a small subsection clearly describing setting the reference for categorical formulas, since this has confused a lot of people (me included) in the past. Should be ready to merge in—thanks @lisa55asil + @mortonjt for your help with this :) |
|
This is looking dope Marcus! I really like the clarification about the
reference for categorical formulas. I added a short sentence that I think
helps make this description more tangible.
…On Tue, Sep 24, 2019 at 2:01 PM Marcus Fedarko ***@***.***> wrote:
@mortonjt <https://github.com/mortonjt> I'm done! Added a small
subsection clearly describing setting the reference for categorical
formulas, since this has confused a lot of people (me included) in the past.
Should be ready to merge in—thanks @lisa55asil
<https://github.com/lisa55asil> + @mortonjt <https://github.com/mortonjt>
for your help with this :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#80?email_source=notifications&email_token=AE6EV4XZ6EMQNRAACDYSSLDQLJ527A5CNFSM4IYSYWZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7PZK6A#issuecomment-534746488>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AE6EV4WIQYV5KM4JVLNRLYLQLJ527ANCNFSM4IYSYWZA>
.
|
-Replace remaining uses of "microbe" with "feature" -Mention that red sea dataset's features are KOs -various other small changes
|
@mortonjt I'm done with editing, so provided you're cool with it I think this is ready to go. Thanks! |
|
Awesome. Thanks @fedarko ! |
An attempt to summarize new stuff:
A lot of changes to the README. I'll summarize a few of the important things here, but it's probably best if you just read over the new README.
Modified the q2 summary visualizations to be a lot easier to interpret:
loglikehood" tolossfor the now-bottom plot, to be consistent with tensorboard--p-summary-intervalif you don't see anything in the plotsRemoved the
--i-feature-tableparameter forqiime songbird summarize-single, since the number of samples is only used to compute Q2 values in the paired summary. (Closes summarize-single doesn't actually need a feature table as input #76.)nan optional parameter for_summarize(). As you can see in the diff, I added some code to check for the never-should-happen case wheren is Nonebutbaseline is not None(i.e. we're making a paired summary but don't have access tonfor computing Q2 values)._summarize()throws an error in this case.All descriptions and defaults for all parameters (of
multinomial, at least) are now stored insongbird/parameter_info.py. This centralized information is used by both the standalone and Q2 versions of Songbird, so now if you want to change a parameter you only need to change one thing.scripts/songbird, so instead of spending a bunch of time manually formatting it I just ran the file throughblack -l 79. (That's why that file looks so different in the diff.)OK, I think that's the bulk of it. Please let me know if you'd like any clarification or for me to change anything. In sum, I think these changes will make using Songbird a lot more pleasant (and understandable) for many people. Thanks!