search

caporaso-lab / sourcetracker2

SourceTracker2
BSD 3-Clause "New" or "Revised" License
60 stars 45 forks source link

St plots #161

Open andy6a opened 4 months ago

andy6a commented 4 months ago

Upgrading plotting function with new plots including Stacked bar and paired heatmap for paired inputs as well as an upgraded heatmap.

cherman2 commented 2 months ago

Hi @andy6a, @colinvwood and I reviewed your changes to this PR. We have some additional comments about code duplication. Let me know if you have any questions!

andy6a commented 2 months ago

Hey @cherman2 @colinvwood I have updated the code to reflect most of the changes asked for. I would appreciate a review of the changes I have made and their use. If they work as planned and as I have tested, I will add an addition to the readme to show the changes with visuals and explanations. The only requested changes I left alone were in reducing code on png file output and in the paired heatmap as I am unsure how to proceed. Let me know what y'all think.

cherman2 commented 2 months ago

Hi @andy6a, Let me know when you want me to re-review this PR

andy6a commented 1 month ago

Hey @cherman2 could you go over the PR again to confirm which things I still need to work through? I know the colors issue is the biggest issue but I need a refresher on what else needs to be done.

cherman2 commented 1 month ago

Hi @andy6a, Let's refocus the discussion here. I think one of the main issues with this PR is how the heatmap.png(s) look for large data.

Here is what I see for the OG heatmap. There is a huge amount of white space around the Heatmap. Also My labels get cut off the left edge of the figure. Otherwise this heatmap is pretty scalable

OG Heatmap todo:

Mixing_Proportions_heatmap

Here is what I see for the paired heatmap. There is also a huge amount of white space around the heatmap that should be removed. Another major problem for scalability is the fact the legend is as long as the figure. For a figure as long as mine this means you aren't able to see the whole range of the legend at once.

Paired Heatmap todo:

Mixing_Proportions_pairedheatmap

Does all this make sense? Let me know if you have any questions or need data to test this on.

andy6a commented 1 month ago

Hey @cherman2, I made some changes which should solve the white space issues, can you test that out on your data. Also might solve label but honestly thats just hopeful thinking. If you could let me know that would be appreciated. (Also what you asked for makes perfect sense.)

cherman2 commented 4 weeks ago

Hi @andy6a, It looks like that did fix the whitespace issue!

Here is my command: sourcetracker2 gibbs -i ~/Downloads/mcallister-shotgun/merged-5-metaphlan.biom -m ~/Downloads/mcallister-shotgun/Mcallister-shotgun-metadata-current.tsv --paired_heatmap --heatmap -o ~/Downloads/st2-plots-test

I think there is two remaining issues:

Sorry for my slow response. This might go faster if you are able to test on your end. Let me know if you need data in order to replicate this.

andy6a commented 4 weeks ago

Hey @cherman2, For the issues you mentioned : I do not have and have not seen a solution to the issue with the legend given that you would have to remove the adaptability of the size to gridspec_kw. Basically, the width_ratios should indicate the width of the bar relative to the other columns. Height_ratios would cause the plots to look goofy when they look smaller. there is a way to add this as a variable but realistically there is no way for a one size fits all by using this. I can add the height ratios to change the height but it would have to be manually altered.

Second, that is intentional because the default for heatmap is true whereas the paired heatmap and stacked bar is false. I can change this if you would like but the heatmap as the default output is a feature I inherited. If you and Dr. Caporaso would like to change that we can but we will need to update that in the readme.

cherman2 commented 1 week ago

Hi @andy6a, re: the size of the legend. This is confusing because the regular heamap scales the legend. I wonder what is included in that plot that is making this possible?

I guess my main issue is that it is confusing that --heatmap does the opposite of --paired-heatmap. I get that its the default but as a user I think its confusing to have these do opposite things while looking like the same flag.

I guess I think that --heatmap should be something like --no-heatmap or something to make it obvious that its going to turn off the default.

andy6a commented 15 hours ago

Hey @cherman2 could you test the stuff I just updated when you have the time and let me know how it works. I took everything from png to svg in the meantime. not hard to go back. Also allow for toggle of the legend.