Closed brett-van-tussler closed 1 year ago
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 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": "dBacteria;pActinobacteriota;cActinomycetia;oPropionibacteriales;fPropionibacteriaceae;gCutibacterium" but opening the html file in a browser shows -
This complicates the testing, any ideas?
@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.
Looks great - thanks so much for this contribution @brett-van-tussler!
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