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 color scheme for pathogenicity for ClinVar config_demo track #3913

Merged
merged 1 commit into from Sep 9, 2023

Conversation

scottcain
Copy link
Member

I thought this was a nice (minor) addition to the clinvar tracks

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Sep 9, 2023
@scottcain scottcain added enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Sep 9, 2023
@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Merging #3913 (a4a4928) into main (da082fc) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3913      +/-   ##
==========================================
- Coverage   64.38%   64.36%   -0.03%     
==========================================
  Files        1004     1004              
  Lines       29866    29866              
  Branches     7158     7158              
==========================================
- Hits        19228    19222       -6     
- Misses      10473    10481       +8     
+ Partials      165      163       -2     

see 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator

cmdcolin commented Sep 9, 2023

looks good :)

image

@cmdcolin
Copy link
Collaborator

cmdcolin commented Sep 9, 2023

could make into a reusable js function perhaps, but works for now

@cmdcolin cmdcolin merged commit cbc2d7f into main Sep 9, 2023
11 checks passed
@cmdcolin cmdcolin deleted the add_clinvar_colors branch September 9, 2023 20:22
@cmdcolin cmdcolin changed the title this color scheme for ClinVar seems pretty common Add color scheme for pathogenicity for ClinVar config_demo track Sep 10, 2023
@cmdcolin cmdcolin added documentation and removed enhancement New feature or request labels Oct 4, 2023
@bbimber
Copy link
Contributor

bbimber commented Oct 15, 2023

@scottcain or @cmdcolin: i was looking over the release notes and came across this. A tangential question: this is an example where a non-trivial JEXL expression is duplicated. JEXL does let one define/register custom functions. Have you considered exposing this in some manner to the session, such that a developer could register custom JEXL expressions with the JBrowse JEXL instance once, and then just reference them in the track config?

@cmdcolin
Copy link
Collaborator

@bbimber not sure if you are using this method or not, but you can use a plugin to register a new jexl function like you refer to https://jbrowse.org/jb2/docs/config_guides/customizing_feature_colors/ you mention exposing it on the session but as the link shows it is exposed on the 'pluginManager'

it would be nice if jexl was a bit more powerful and you could e.g. store the get(feature,'INFO').CLNSIG in a variable in this example, but making the custom plugin with custom jexl functions is sort of the current workaround

@cmdcolin
Copy link
Collaborator

also, if you are using the embedded, you do not even need to write a plugin in this case, as you can access the pluginManager with something like this wherever you want in your code

const {pluginManager}=getEnv(session) // or getEnv(viewModel), getEnv(displayModel), etc. getEnv works on any model because every model has the same env with the pluginManager, it is "globally available" via the env

@bbimber
Copy link
Contributor

bbimber commented Oct 16, 2023

@cmdcolin, ok, thanks for the reply. we are doing something similar in our embedded case (which is using plugins for everything). I was musing above about whether sharing of non-trivial JEXL expressions is possible, but it seems like plugin is a reasonably minimal way to do this.

@cmdcolin
Copy link
Collaborator

oh right. I think I would aim to make non-trivial stuff into js callbacks via the plugin method. this example in this PR would probably benefit from being made into a small callback :) ya, i see you maybe were referring to sharing actual pure jexl expressions across multiple callbacks, but i think just going straight to the custom jexl functions is maybe what i'd do

@bbimber
Copy link
Contributor

bbimber commented Oct 16, 2023

oh right. I think I would aim to make non-trivial stuff into js callbacks via the plugin method. this example in this PR would probably benefit from being made into a small callback :) ya, i see you maybe were referring to sharing actual pure jexl expressions across multiple callbacks, but i think just going straight to the custom jexl functions is maybe what i'd do

The impetus for starting this thread was noticing a long JEXL repeated twice in the same JSON config file of this commit, related to color. If the JSON config had some kind of hook to register JEXL (and it seems like registering a custom plugin is the best current option), then that duplication could have been avoided. In theory the JSON config could be extended to more directly let a developer define/register JEXL, but given what's possible I'm not sure that's needed.

That's all I was asking about, and the answers here have been helpful.

@scottcain
Copy link
Member Author

Did I copy and paste code on a quicky change to make my life easier? yes. Should I have? Nope!

@bbimber
Copy link
Contributor

bbimber commented Oct 16, 2023

Did I copy and paste code on a quicky change to make my life easier? yes. Should I have? Nope!

not meant as any sort of a critique...just trying to understand capabilities of jbrowse. it's all a balance of time and effort.

@scottcain
Copy link
Member Author

I didn't take it as one; I just realize that a config like this is really a teaching tool so I should have thought of that. Do I copy and paste jexl in my own configs: absolutely :-)

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

Successfully merging this pull request may close these issues.

None yet

3 participants