Skip to content
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

St plots #161

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

St plots #161

wants to merge 55 commits into from

Conversation

andy6a
Copy link
Contributor

@andy6a andy6a commented Mar 4, 2024

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

sourcetracker/_cli/gibbs.py Outdated Show resolved Hide resolved
sourcetracker/_cli/gibbs.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
@cherman2 cherman2 self-requested a review April 22, 2024 21:12
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
@andy6a
Copy link
Contributor Author

andy6a commented May 3, 2024

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.

Copy link
Collaborator

@cherman2 cherman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy6a
It looks like the two heatmap vizs and two bar plots might be able to merge into one viz respectively.

There is alot of code duplication still, I tried to add code suggestions as examples for how to address this.

sourcetracker/_plot.py Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_gibbs_defaults.py Outdated Show resolved Hide resolved
sourcetracker/_cli/gibbs.py Outdated Show resolved Hide resolved
Comment on lines 252 to 258
"""
# '#1f77b4'Blue, '#ff7f0e'Orange, '#2ca02c'Green, '#d62728'Red,
# '#9467bd'Purple, '#8c564b'Brown, '#e377c2'Pink, '#7f7f7f'Grey,
'#bcbd22'Gold, '#17becf'Cyan
#make sure to use contrasting colors in order better illuminate
your data above are some example codes to use
"""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think this should be removed or moved up to top of method.

sourcetracker/_plot.py Outdated Show resolved Hide resolved
ax=axes[i])
g[i].set_xlabel("")
g[i].set_ylabel("")
g[i].set_yticks([])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you need to address this?

sourcetracker/_plot.py Outdated Show resolved Hide resolved
sourcetracker/_plot.py Outdated Show resolved Hide resolved
@cherman2
Copy link
Collaborator

cherman2 commented May 7, 2024

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

@andy6a
Copy link
Contributor Author

andy6a commented May 23, 2024

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
Copy link
Collaborator

cherman2 commented May 28, 2024

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:

  • remove top white space
  • remove bottom white space
  • Don't cut off x-labels

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:

  • Remove top white space
  • remove bottom white space
  • Don't cut off x-labels
  • make the legend fix size like in OG-heatmap.

Mixing_Proportions_pairedheatmap

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

@andy6a
Copy link
Contributor Author

andy6a commented May 28, 2024

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
Copy link
Collaborator

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:

  • The legend for your paired heatmap is still as long as the figure
  • The command above only give me a paired_heatmap. I believe that this should give me a paired heatmap and a heatmap. I think passing the flag for a viz should always give me that visualization.

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
Copy link
Contributor Author

andy6a commented Jun 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants