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

monarchr 1.5.1 #52

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

monarchr 1.5.1 #52

wants to merge 4 commits into from

Conversation

bschilder
Copy link
Contributor

@bschilder bschilder commented Jan 8, 2025

monarchr 1.5.1

New features

@oneilsh
Copy link
Collaborator

oneilsh commented Jan 13, 2025

@bschilder these are wonderful! Some thoughts:

  • This is at least a minor bump to 1.6.0!

  • In a local branch I am working on incorporating the Monarch semantic similarity search endpoint, which is different than the monarch_semsim (which does nodeset-vs-nodeset semantic matching). So I am planning to deprecate monarch_semsim renaming it to monarch_simsim_match, and add monarch_semsim_search. The search endpoint is pretty slow, so I haven't committed to adding it to the package yet. Do you think graph_semsim would be clear to users in that world? I see the other utility functions you added have a graph_* naming scheme which is nice continuity. Should sparsity be graph_sparsity to match?

  • In another local branch I put together descendants() and ancestors() as convenience wrappers around expand() with transitive = TRUE. Once we merge yours I'll modify them to take an n argument and use your iterative expand_n. More of an FYI :) Doing iterative transitive expansion that way will be slower than full, since for the Neo4j engine it is done in one shot via the cypher query. Actually, now that I think about it, you may want to remove the transitive parameter from expand_n, since when passed to expand() it will fetch the complete transitive set (same for file engines), which might not align with user expectations. I like the example you show of not using transitive but using biolink:subclass_of with expand_n to fetch a limited transitive set.

  • I am wondering if the Monarch-specific attributes and values in kg_edge_weights should be stored in the kg_prefs.yaml file where I've been putting other defaults that are specific to the Monarch KG. Feasible or inadvised?

  • Similarly, some of the monarch-specific palette and theme info could be stored in the kg_prefs.yaml too. They are nicely wrapped up in monarch_palettes() and theme_monarch(), so maybe just some of the info currently hard-coded in monarch_palettes()?

  • The new visualizations vignette seems to be a work in progress - it has some huge queries and I think is breaking the checks. Can we move it out of the build till more baked? The vignettes/examples/visualizing_kgs.Rmd will need a small update to drop the old edge_color parameter.

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

Successfully merging this pull request may close these issues.

2 participants