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

Add gene_labels and labels to plot_genes() #653

Merged
merged 11 commits into from
Dec 3, 2024
Merged

Conversation

leehart
Copy link
Collaborator

@leehart leehart commented Nov 14, 2024

Resolves #407.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@leehart leehart requested a review from alimanfoo November 18, 2024 09:22
@alimanfoo
Copy link
Member

Looking good...

image

@alimanfoo
Copy link
Member

Hi @leehart, couple of comments...

One very small request, would it be possible to add just a little more vertical padding around the text labels? Particularly the negative strand gene labels are touching the gene rectangles, would be good to have a little padding.

The other request is that ultimately this will be needed as part of functions like plot_h12_gwss() where there is a genes track plotted below some other data track. So basically, could you add a gene_labels parameter to all functions like this, i.e., where plot_genes() is called internally?

@leehart leehart marked this pull request as draft November 21, 2024 12:36
@leehart
Copy link
Collaborator Author

leehart commented Nov 22, 2024

@alimanfoo How does this padding look to you?
Screenshot 2024-11-22 at 12 56 18

Ideally I'd like a more robust mechanism for accommodating these labels, because this is currently tailored to the default height of the plot, i.e. 120, so specifying a different height, e.g. height=240, results in a skewed layout, e.g.
Screenshot 2024-11-22 at 13 02 44

@leehart leehart marked this pull request as ready for review November 22, 2024 15:53
@alimanfoo
Copy link
Member

alimanfoo commented Nov 27, 2024

@alimanfoo How does this padding look to you? Screenshot 2024-11-22 at 12 56 18

Looks good!

Ideally I'd like a more robust mechanism for accommodating these labels, because this is currently tailored to the default height of the plot, i.e. 120, so specifying a different height, e.g. height=240, results in a skewed layout, e.g. Screenshot 2024-11-22 at 13 02 44

Thanks @leehart, I think it's probably ok for now, generally unlikely to need to vary the height of this track by much.

@alimanfoo
Copy link
Member

Hi @leehart, could we rename the labels param to gene_labelset? That way when the parameter appears in functions like plot_h12_gwss() it will be more obvious that the parameter will apply to the genes track.

@leehart leehart marked this pull request as draft November 28, 2024 09:44
@leehart leehart marked this pull request as ready for review November 28, 2024 10:19
Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 28 lines in your changes missing coverage. Please review.

Project coverage is 94.57%. Comparing base (af6bac1) to head (cc1ce67).
Report is 79 commits behind head on master.

Files with missing lines Patch % Lines
malariagen_data/anoph/genome_features.py 6.66% 28 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   95.19%   94.57%   -0.62%     
==========================================
  Files          40       40              
  Lines        4144     4187      +43     
==========================================
+ Hits         3945     3960      +15     
- Misses        199      227      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leehart
Copy link
Collaborator Author

leehart commented Nov 29, 2024

Looks like codecov/patch wants some test coverage for the new gene_labels param. Will do.

@leehart leehart marked this pull request as draft November 29, 2024 17:46
@alimanfoo
Copy link
Member

Looks like codecov/patch wants some test coverage for the new gene_labels param. Will do.

Cool, thanks @leehart. Happy to be merged once codecov/patch passes.

@leehart leehart marked this pull request as ready for review December 3, 2024 10:34
@leehart leehart merged commit 6c27d20 into master Dec 3, 2024
10 checks passed
@leehart leehart deleted the GH407_allow_gene_labels branch December 3, 2024 10:54
@alimanfoo alimanfoo added the BMGF-068808 Work supported by BMGF grant INV-068808 (MalariaGEN 2024-2027). label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BMGF-068808 Work supported by BMGF grant INV-068808 (MalariaGEN 2024-2027).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label genes
2 participants