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

Edge encoding #15

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

Edge encoding #15

wants to merge 14 commits into from

Conversation

jjustison
Copy link
Member

See previous pull request here. PR'ing form this branch for more public access to modify code

@jjustison
Copy link
Member Author

I just saw that the dev branch was made and had changes to it. I will merge these two branches and then perhaps it will be worthwhile to look more at this request

Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines Coverage Δ
src/PhyloCoalSimulations.jl 100.00% <ø> (ø)
src/simulatecoalescent_network.jl 100.00% <100.00%> (ø)
src/simulatecoalescent_onepop.jl 100.00% <100.00%> (ø)
src/utils.jl 100.00% <100.00%> (ø)

Also removed the Project file in the test folder and tried to add them as  in the main Project file. The only complication came from adding  as a test-only dependency. I added the development version directly from url. However, when trying to remove SNaQ from  I would get an error when running tests saying that it was expecting SNaQ to be registered.

I think(?) this can be resolved by adding the main branch of SNaQ when those changes are finally pushed and registered.
@jjustison
Copy link
Member Author

I was working on the pull request for PhyloCoalSimulations and ending up fixing some of the tests as part of the process. I have those changes in the edge_encoding branch where there is a pull request. This branch also currently has the most up to date changes merged from the dev
branch.

The only funky thing that I couldn't quite figure out was how to properly have SNaQ as a test-only dependency. I added the dev version of SNaQ as a dependency. It is only needed in the tests but whenever I try to remove it from [deps] in the Project file, I get the following error:
expected package SNaQ [c2bf7a07] to be registered.
Tests pass locally with how I have SNaQ set up but don't pass on CI.

@cecileane
Copy link
Member

It would be so much better to not depend on SNaQ, even for tests.
What would you think of a small package for quartet concordance factor utilities, separate from SNaQ? I know it's more work, but I've come across this need multiple times now.

@jjustison
Copy link
Member Author

I suppose it might depend on what all is included but if they are somewhat general utilities that come up in a few places, then perhaps they could go in PhyloNetworks? Otherwise, if it ends up being a large amount of utilities, and datatypes, then maybe a new package might make sense.

I don't take huge issue with depending on SNaQ for only tests but I could see it being unideal in other cases where packages have it as a dependency for only the few CF-related functions.

@cecileane
Copy link
Member

I thought that having a separate project file test/Project.toml would make it easier to declare SNaQ as a dependency of running tests, but not a dependency of the package itself. I am trying strategies to limit the dependency of SNaQ to tests only. I hope to push an option soon.

@cecileane
Copy link
Member

I hope all checks will pass, with SNaQ removed from the package's dependencies (but included for tests). The documentation should pass after PhyloPlots v2.0 is officially registered.

In my last commit, I exported the function population_mappedto and modified examples to promote the use of this function to access the embedding of gene trees into the species network, and to discourage the use of the internal field .inte1 (or whatever it is).

I wonder the new function encode_edges! could have a more explicit name, to refer to edges in gene trees (vs. species phylogeny) and to refer to a "mapping" or "embedding" inside species or populations, to make "encoding" more explicit. Perhaps embed_geneedges! ? or population_mapping! (to highlight the similarity to population_mappedto ? other ideas?

@cecileane
Copy link
Member

I suppose it might depend on what all is included but if they are somewhat general utilities that come up in a few places, then perhaps they could go in PhyloNetworks? Otherwise, if it ends up being a large amount of utilities, and datatypes, then maybe a new package might make sense.

I don't take huge issue with depending on SNaQ for only tests but I could see it being unideal in other cases where packages have it as a dependency for only the few CF-related functions.

I agree. For now we can keep SNaQ as a dependency for tests. Independently, we can list the general utilities that come up in several places that don't need the snaq! estimation method. We've recently seeing many people and methods that can use quartet concordance factors as input, so utilities to read qCF data and calculate qCFs from gene trees would be part of this list of general utilities.

@jjustison
Copy link
Member Author

I hope all checks will pass, with SNaQ removed from the package's dependencies (but included for tests). The documentation should pass after PhyloPlots v2.0 is officially registered.

In my last commit, I exported the function population_mappedto and modified examples to promote the use of this function to access the embedding of gene trees into the species network, and to discourage the use of the internal field .inte1 (or whatever it is).

I wonder the new function encode_edges! could have a more explicit name, to refer to edges in gene trees (vs. species phylogeny) and to refer to a "mapping" or "embedding" inside species or populations, to make "encoding" more explicit. Perhaps embed_geneedges! ? or population_mapping! (to highlight the similarity to population_mappedto ? other ideas?

I think perhaps we could combine both of those ideas into something like gene_edgemapping!. I think this combines the idea that it is something for the gene tree edges while also evoking similarity to population_mappedto. Otherwise I also just like embed_geneedges!, it is short and more explicit than encode_edges!

@cecileane cecileane self-requested a review November 26, 2024 22:23
@cecileane
Copy link
Member

Let's wait until PhyloPlots v2.0 is released, then re-run the documentation to make sure it builds. I don't understand what's holding back the release of PhyloPlots...

@cecileane cecileane marked this pull request as draft December 3, 2024 15:24
@cecileane cecileane marked this pull request as ready for review December 10, 2024 22:35
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