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

🎨 Only map synonyms when field is name #2312

Merged
merged 3 commits into from
Jan 2, 2025
Merged

🎨 Only map synonyms when field is name #2312

merged 3 commits into from
Jan 2, 2025

Conversation

sunnyosun
Copy link
Member

@sunnyosun sunnyosun commented Jan 2, 2025

Prevents mistakes like: #2137

Previously, synonym mapping was always performed no matter which field it was in.

This PR makes sure that synonyms are only mapped on the name field, e.g., bt.Gene.symbol

Before After
Screenshot 2025-01-02 at 21 57 05 Screenshot 2025-01-02 at 21 58 09

Copy link

codecov bot commented Jan 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.64%. Comparing base (2a17099) to head (ffa5fb1).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2312      +/-   ##
==========================================
- Coverage   92.82%   85.64%   -7.19%     
==========================================
  Files          55       81      +26     
  Lines        7202     8559    +1357     
==========================================
+ Hits         6685     7330     +645     
- Misses        517     1229     +712     

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

Copy link

github-actions bot commented Jan 2, 2025

@github-actions github-actions bot temporarily deployed to pull request January 2, 2025 11:55 Inactive
@github-actions github-actions bot temporarily deployed to pull request January 2, 2025 12:33 Inactive
@falexwolf
Copy link
Member

Great! Just can you add more color to the description of the PR with a small example so that everybody reading this understands what's changing? It's even quite hard for me (even though I figured it out). One reason is also that the PR title isn't descriptive enough. Can you please improve it?

Maybe just the one you mentioned here: #2137 (comment)

@sunnyosun sunnyosun merged commit a4ea5be into main Jan 2, 2025
16 of 17 checks passed
@sunnyosun sunnyosun deleted the standardize-name branch January 2, 2025 12:49
@falexwolf
Copy link
Member

Thanks for the example!

But: What about the incomprehensible PR title that will also appear in the changelog?

And: What about the grammatical error in this sentence: "This PR makes sure that standardize only performs on name field."

And: What does the example illustrate? I mean, what was the behavior before and what after the change, @sunnyosun?

@sunnyosun sunnyosun changed the title 🎨 Standardize name 🎨 Only map synonyms is field is name Jan 2, 2025
@sunnyosun sunnyosun changed the title 🎨 Only map synonyms is field is name 🎨 Only map synonyms when field is name Jan 2, 2025
@sunnyosun
Copy link
Member Author

The example shows synonyms are only mapped when field is "symbol", not "ensembl_gene_id". I don't think "This PR makes sure that standardize only performs on name field." has grammar error.

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