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

updates for metadata and manuscript finalization #2

Closed
wants to merge 8 commits into from

Conversation

gregcaporaso
Copy link
Member

No description provided.

if `export_legend` is enabled, present the legend in the resulting
Visualization.
Note that there was a minor bug in the original version of the
Visualizer - since the conditional was being evaluated after
`export_legend` was cast to a string, it always evaluated to `True`.
@gregcaporaso
Copy link
Member Author

@lizgehret, this should be updated for the new metadata. Here's the bucket 3/4 plot that you create in your example command - does this align with your previous version?

Figure:
image

Legend:
image

@lizgehret
Copy link
Contributor

Hey @gregcaporaso, I think there's a little confusion around the export_legend functionality - it should only be included in the main plot if export_legend is set to False. The purpose of this is to allow for Jeff to combine multiple bucket figures into a faceted view in biorender, with the legend exported as a separate figure that he can add in where it stylistically makes sense. The expected behavior is as follows:

if export_legend == False:
Only the pcoa_plot.png should be included in the data directory, and it should include the legend in the main figure.
if export_legend == True:
The pcoa_plot.png and legend.png should both be included in the data directory, and the legend should not be included in the main figure.

I just tested this again and confirmed it's working as expected. Do you think it would be helpful to change the parameter name to prevent any confusion on what the expected behavior is?

@lizgehret
Copy link
Contributor

lizgehret commented Oct 8, 2024

Re: the figure looking the same - it looks very similar, but not exactly identical. Here's what my version looks like:
pcoa_plot

I'm just seeing a few differences in the bulking material samples, which we may want to look at together during the composting meeting (just to make sure none of the Metadata changes unintentionally changed the identification of any particular samples).

Copy link
Contributor

@lizgehret lizgehret left a comment

Choose a reason for hiding this comment

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

i took a closer look at the export_legend stuff we were discussing @gregcaporaso - here are more detailed comments.

gut_to_soil_manuscript_figures/_methods.py Outdated Show resolved Hide resolved
gut_to_soil_manuscript_figures/_methods.py Outdated Show resolved Hide resolved
gut_to_soil_manuscript_figures/_methods.py Outdated Show resolved Hide resolved
gut_to_soil_manuscript_figures/_methods.py Outdated Show resolved Hide resolved
@@ -62,8 +63,7 @@ def _bucket_util(highlighted_buckets, md, ord_2d):
# HE
bucket_ids_HE_week0 = \
md[(md['Bucket'] == bucket) &
(md['SampleType2'] == 'Self Sample') &
(md['Week'] == 0.0)].index.values
(md['SampleType'] == 'Human Excrement')].index.values
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the (md['Week'] == 0.0)].index.values got removed. Was this intentional?

@@ -73,17 +73,16 @@ def _bucket_util(highlighted_buckets, md, ord_2d):
# bulking
bucket_ids_bulk_week0 = \
md[(md['Bucket'] == bucket) &
(md['SampleType2'] == 'Bulking Material') &
(md['Week'] == 0.0)].index.values
(md['SampleType'] == 'Bulking Material')].index.values
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, it looks like (md['Week'] == 0.0)].index.values was removed here as well.

@@ -233,8 +235,7 @@ def plot_pcoa_2d(metadata_fp, ordination_fp, measure,
if not highlighted_buckets:
# HE wk 0 mean
HE_week0 = \
md[(md['SampleType2'] == 'Self Sample') &
(md['Week'] == 0.0)].index.values
md[(md['SampleType'] == 'Human Excrement')].index.values
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re: (md['Week'] == 0.0)].index.values

@@ -245,8 +246,7 @@ def plot_pcoa_2d(metadata_fp, ordination_fp, measure,

# bulk wk 0 mean
bulk_week0 = \
md[(md['SampleType2'] == 'Bulking Material') &
(md['Week'] == 0.0)].index.values
md[(md['SampleType'] == 'Bulking Material')].index.values
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re: (md['Week'] == 0.0)].index.values

@gregcaporaso
Copy link
Member Author

@lizgehret, thanks for catching the Week 0.0 issue! I think I fixed it now - we replaced those with empty cells, so checks for week == 0.0 are now checks for Composting Time Point is np.nan.

Here's the broken version I had yesterday for bucket 7:
image

Here's today's fixed version:
image

And here is one I'm pulling from Basecamp which I believe is the target:
Screenshot 2024-10-09 at 7 14 33 AM

Note that in our metadata clean-up process, a few of the FLWC samples were removed so that is almost certainly the source of minor differences in the ordination.

@lizgehret
Copy link
Contributor

@gregcaporaso this does look better - the only thing I'm wondering about is the number of FLWC samples that are in your updated version (in comparison to the version from BC). It appears there's a higher number of that sample type than there was before - is that expected? We may want to do a quick tally of the total number we're expecting and visually see if that matches up with the updated plot.

@gregcaporaso
Copy link
Member Author

Replaced by #4

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.

2 participants