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.

Sign up for GitHub

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

Jump to bottom

enable user to increase the viewable length of the y-axis labels #125

Merged
merged 5 commits into from
Jul 27, 2023
Merged

enable user to increase the viewable length of the y-axis labels #125

merged 5 commits into from
Jul 27, 2023

Conversation

brett-van-tussler
Copy link
Contributor

@brett-van-tussler brett-van-tussler commented Jul 13, 2023 •
edited by gregcaporaso
Loading

Added label_limit parameter to y-axis of da_plot. Move y_axis label to the top of the plot. This change now requires python library 'typing' to be installed, which I believe is installed with qiime2

@gregcaporaso The label limit has a default of None, which required using the typing python package. I believe this in installed with qiime2, but I am not sure how to check.

Here is the zip file, I edited the file name to be able to upload to github, and a screen shot just in case.

Also, what does PR mean?
Thanks!

q2_update_da_plot.zip
image

Added label_limit parameter to y-axis of da_plot. Move y_axis label t…
661bc5c 
…o the top of the plot. This change now requires python library 'typing' to be installed
gregcaporaso
gregcaporaso requested changes Jul 13, 2023
View reviewed changes
q2_composition/_diff_abundance_plots.py Outdated
@@ -138,7 +146,8 @@ def da_barplot(output_dir: str,
significance_threshold: float = 1.0,
effect_size_threshold: float = 0.0,
feature_ids: qiime2.Metadata = None,
level_delimiter: str = None):
level_delimiter: str = None,
label_limit: Optional[int] = None):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think Optional is necessary here. Have you tried the following:

Suggested change
label_limit: Optional[int] = None):
label_limit: int = None):

Providing a default should make it optional (this is what we do for several of the parameters above).

q2_composition/_diff_abundance_plots.py Outdated
@@ -125,6 +128,11 @@ def _plot_differentials(
cornerRadius=10,
)

chart = chart.configure_axisY(titleAlign='left', titleY=-10, titleAngle=0)
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this into the new if block below? I don't think it's needed by default (correct me if I'm wrong about that).

Can you also include a comment above this line describing why these modifications are needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not required, it just changes the way the plot axis label looks. Maybe worth keeping outside? For now, I will move it into the if statement.

Copy link
Contributor Author

@brett-van-tussler brett-van-tussler Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Image without the updated line, followed by image with the updated line.
image
image

Copy link
Member

@gregcaporaso gregcaporaso Jul 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I generated these with and without that, and I'm good with keeping it where you had it. It's more readable this way.

I pushed up a commit to move this back to where you had it. I also changed the y-axis title so it says "Feature ID" instead of "Feature ID (most specific, if taxonomic)". The parenthetical information is no longer relevant, and the label fits better at the top with this change (when the feature ids are shorter).

q2_composition/plugin_setup.py Outdated
@@ -198,7 +199,8 @@
"this index."),
'level_delimiter': ("If feature ids encode hierarchical information, "
"split the levels when generating feature labels "
"in the visualization using this delimiter.")},
"in the visualization using this delimiter."),
'label_limit': ("Set labelLimit for y-axis labels")},
Copy link
Member

Choose a reason for hiding this comment

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

Remember that this documentation is intended for end users, not developers. In most cases end users won't know what labelLimit is, so your documentation should describe the effect that setting this parameter will have rather than what is technically happening behind the scenes.

Suggested change
'label_limit': ("Set labelLimit for y-axis labels")},
'label_limit': ("Set the maximum length that will be viewable for axis labels. "
"You can set this parameter if your axis labels are being cut off.")},

@gregcaporaso gregcaporaso changed the title Added label_limit parameter to y-axis of da_plot. Move y_axis label t… enable user to increase the viewable length of the y-axis labels Jul 13, 2023
@gregcaporaso
Copy link
Member

Thanks for the pull request @brett-van-tussler! (PR == pull request, sorry for the unexplained jargon!)

I have a couple of comments, and tests are currently failing due to a lint error. To perform linting locally, you can install flake8. Then change to the top-level directory for this repository (q2-composition) and run flake8 .. That will spit out any violations of the style guide, and you can address those and re-run until you get no errors.

Can you also add a unit test for this new functionality? The easiest way to do that would be to copy one of the existing related tests, eg this one, rename it (e.g., test_label_limit), and adapt the test function to test the new functionality. In this case, you could toggle the new parameter and confirm that the labels end with ellipses when the parameter is not provided and that the labels are full-length when the parameter is provided. (I think that's how it works, but you may need to adapt somehow if the labels are always present in full. Just let me know if you need input.)

@gregcaporaso gregcaporaso mentioned this pull request Jul 6, 2023
Open
@brett-van-tussler
Copy link
Contributor Author

brett-van-tussler commented Jul 19, 2023 •
edited
Loading

@gregcaporaso It looks to me like the graphs decide how much of the label to cut off upon generation. I have attached the html file (renamed to allow upload)
body_habitatUBERON%3Afeces-ancombc-barplot.txt

In the datasets, the full taxonomic assignment is there - "datasets": {"data-e916d9a522633a1ba63c02ad53bed60e": [{"id": "d__Bacteria;p__Actinobacteriota;c__Actinomycetia;o__Propionibacteriales;f__Propionibacteriaceae;g__Cutibacterium" but opening the html file in a browser shows -
image

This complicates the testing, any ideas?

@gregcaporaso
Copy link
Member

@brett-van-tussler, it looks like you can test for the presence of the label limit. For example, if you set it to 4242, you should have {"labelLimit": 4242} in the html file.

Note: I added a commit to this branch (see my comment above) so be sure to git pull before doing more work on this.

Also, we are behind in our release schedule as one of our developers is out sick this week. So if you get this additional test in in the next few days we should still be able to get this in the 2023.7 release.

@gregcaporaso gregcaporaso merged commit 6d69fd4 into qiime2:master Jul 27, 2023
3 checks passed
@gregcaporaso
Copy link
Member

Looks great - thanks so much for this contribution @brett-van-tussler!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregcaporaso gregcaporaso gregcaporaso approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
2023.7
Status: Completed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brett-van-tussler @gregcaporaso