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.

Sign up for GitHub

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

Jump to bottom

Flip Hybrid Edge Move #139

Merged
merged 49 commits into from
Oct 23, 2020
Merged

Flip Hybrid Edge Move #139

merged 49 commits into from
Oct 23, 2020

Conversation

coraallensavietta
Copy link
Collaborator

@coraallensavietta coraallensavietta commented Sep 8, 2020 •
edited
Loading

  • add new structure optimization move to flip a hybrid edge's direction with careful attention to new root selection if hybrid edge to flip is below the root

@coraallensavietta
updated move percentages, invariable site percentage
f2d93ff 
drafting user tutorial

increase add hybrid moves, decrease delete hybrid moves

more info on site variability
@coraallensavietta
docs drafting
e636ee2
@coraallensavietta
clean up showdata
d544b5b
@coraallensavietta
option to not remove ghost hybrids (unfinished), start of optimize pa…
316ff3b 
…rameter test
@coraallensavietta
cleaned up optimizeparamters and tests
619800d
@coraallensavietta
drafted neighborNets function to see all neighbors using NNIs
aef94ef
@coraallensavietta
neighborsNets
2ca3830
@coraallensavietta
uniqueneighbornets
46e8e66
@coraallensavietta
started function to count number of NNIs to true network
804a953
@coraallensavietta
tests for nnistotruenet
f9162a3
@coraallensavietta
improved and checked neighbornets and uniqueneighbornets
8914b17
@coraallensavietta
cleaned up nni count function
20332e5
@coraallensavietta
added function to test how nnis affect hybrid edges, removed repetiti…
c7f78fc 
…ve debug message in auxillary
@coraallensavietta
small bug fix in test
017933e
@coraallensavietta
new version of nnistotruenet that moves only uphill, following non-de…
0d1bcf7 
…creasing likelihoods
@coraallensavietta
finishing hill-climbing nni move counter and tests
f1b2bbe
@coraallensavietta
closer optimization in optimizeparameters, more messages in hill-clim…
5886ddb 
…bing nnistotruenet to catch places where the true net has a lower loglik.
@coraallensavietta
update hybridschangedbynnis to not rely on node numbers to identify f…
21b5a83 
…lipped hybrids
@coraallensavietta
better hybrid flip test, but only for networks with one hybrid
6314b52
@coraallensavietta
new debug statements, added gamma optim in optimizeparameters, cleane…
01dcff2 
…d up hybridschanged function
@coraallensavietta
start on new hybrid flip move, additional debug statements in optimiz…
470a66e 
…elength
@coraallensavietta
new test for W structure, fix typo
7bc8ed5
@coraallensavietta
start on modify in place hybrid flip, notes on optimizelength bug
d8404ad
@coraallensavietta
new fliphybrid notes
3905256
@coraallensavietta coraallensavietta marked this pull request as ready for review October 13, 2020 19:21
@cecileane
Copy link
Member

cecileane commented Oct 13, 2020 •
edited
Loading

Did you mean to commit the files examples/h1*? examples/h1_net.fasta is quite large (for an example file --not for a true data file). If this is for the manual, are you sure it's not too large to run fast?

@coraallensavietta
Copy link
Collaborator Author

Did you mean to commit the files examples/h1*? examples/h1_net.fasta is quite large (for an example file --not for a true data file). If this is for the manual, are you sure it's not too large to run fast?

That's a great point! They are for the manual, so I'll remove them from this branch, then make a note to myself to confirm that they're not too long for the manual before I add them into that PR.

@cecileane
bug fix, doc & comments, nohybridladder=false by default
08823ac 
in generic flip hybrid (still true by default in PhyLiNC)
bug fix: flip may be admissible even downstream of hybrids
bottom rung of ladder: more efficient check
@cecileane
Merge branch 'master' (with tree-edge components) into optim
b3e7f85
@cecileane
Copy link
Member

cecileane commented Oct 15, 2020 •
edited
Loading

I'm happy with the code, at this point. I think that:

  • we should go through the debug statements to see which ones are "low-level", that were used during debugging for specific things, and that should be commented out. They should be kept if they are useful at a higher level, to follow the general flow of phylinc.

  • we could increase test coverage for the phylinc function. The one test case starts with a standard network. Could you possibly use a different starting network, like the "w-structure" network in the other test file, with 2 reticulations and the w-structure on which none of the "minor" flips are admissible (under the nohybridladder contraints)? That could help cover more of the code. Also,

    • if the starting network could have "good" branch lengths (optimized ahead of time outside of the test on that fixed topology) and
    • if the lcache and the phylinc tolerance parameters could be very sloppy (bump them up from 1e-6 to 1e-2 or higher)

    so that the move doesn't increase the likelihood, that would be good: to cover the code to undo the flip.

@cecileane
Copy link
Member

Hi Cora, did you notice that the tests failed, despite the "all good" green light on GitHub? see here.

@coraallensavietta
Copy link
Collaborator Author

Interesting! I can replicate these errors with Julia 1.5, but not 1.4. I'll look into it.

@cecileane
Copy link
Member

Everything looks good to me. If same for you, you can squash & merge @coraallensavietta . Thank you!!

@coraallensavietta coraallensavietta merged commit 6b0e0e0 into master Oct 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cecileane cecileane cecileane approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@coraallensavietta @coveralls @cecileane