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

Minor fixes: added 2 spaces and QuartetNetwork docstring #135

Merged
merged 7 commits into from
Sep 7, 2020

Conversation

crsl4
Copy link
Member

@crsl4 crsl4 commented Aug 3, 2020

  • Adding 2 spaces that were removed by emacs in this commit
  • Created the docstring for QuartetNetwork

@crsl4 crsl4 requested a review from cecileane August 3, 2020 19:35
@cecileane
Copy link
Member

cecileane commented Aug 3, 2020

This is unrelated to the docstring, but it would be good to include a formal test for the changes to writeTopologyLevel1 and to the end of snaq!, in 1da3325 .

By the way, the chunk of code repeated in writeTopologyLevel1 and in snaq! could possibly be moved to checkRootPlace!: There would be less room for bugs (e.g. bug fixed in one place but not in the other, when the chunk is repeated), and the code for both writeTopologyLevel1 and in snaq! would be cleaner. The formal test could also be made cleaner that way.

@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #135 into master will increase coverage by 0.21%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
+ Coverage   84.35%   84.56%   +0.21%     
==========================================
  Files          28       28              
  Lines       10311    10770     +459     
==========================================
+ Hits         8698     9108     +410     
- Misses       1613     1662      +49     
Impacted Files Coverage Δ
src/PhyloNetworks.jl 100.00% <ø> (ø)
src/addHybrid.jl 96.51% <ø> (+5.04%) ⬆️
src/manipulateNet.jl 89.22% <ø> (+0.06%) ⬆️
src/substitutionModels.jl 93.24% <ø> (+0.02%) ⬆️
src/types.jl 80.64% <ø> (-4.11%) ⬇️
src/snaq_optimization.jl 74.95% <50.00%> (+0.36%) ⬆️
src/readwrite.jl 73.39% <100.00%> (+4.74%) ⬆️
src/traits.jl 96.05% <100.00%> (+0.40%) ⬆️
src/moves_snaq.jl 82.84% <0.00%> (-1.90%) ⬇️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f122d0c...9a31f17. Read the comment docs.

@crsl4
Copy link
Member Author

crsl4 commented Aug 3, 2020

yes, I agree! There is a lot of things that I want to re-write on this checkRoot situation. It is in my to-do list, but it needs some time to sit down and focus that I have not had. I will put my notes on an issue.

@coveralls
Copy link

coveralls commented Aug 3, 2020

Coverage Status

Coverage increased (+0.2%) to 84.568% when pulling 9a31f17 on minor-fixes into f122d0c on master.

@cecileane
Copy link
Member

just to see how coverage had decreased after adding the two chunks to check that root is admissible: on codecov

@cecileane
Copy link
Member

cecileane commented Sep 5, 2020

To get more detail on code that was added but is not covered during testing: see in readwrite.jl and in snaq_optimization.jl. The code coverage is really poor for some files, so I find it important to do better with new code.

I need to fix a bug in the function that direct edges, because that function should return an error if the network is not a DAG. I would like to work from a clean master branch with this current PR finished. Could you please help @crsl4?

@cecileane
Copy link
Member

We also need to take care of the DataStructure update (PR #137). I would love for this PR to be finished, and make progress on the other things.

@crsl4
Copy link
Member Author

crsl4 commented Sep 5, 2020

Sorry Cecile! I was waiting for you to merge the pull request. I got lost whether there was something missing here. By reading again, are we mainly missing the writeTopologyLevel1 tests? Do we want to include them in this PR or could we merge the PR and do the tests later?

@cecileane
Copy link
Member

Yes we should include them in this PR. Otherwise, we will never do it. We should also fix the documentation errors (Travis says "it's all good" in its summary, but when we look at what happened, there were errors.) I'm working on fixing the doc errors right now.

@cecileane
Copy link
Member

It looks like the random number generator has changed, in julia v1.4 vs v1.5... causing more errors in the doc. and probably a lot more in the tests...

- stick to 80-character lines in markdown docstring
- example using random seeds modified (julia v1.5)
- tests & doc build under julia v1.5 and Documenter v0.25
- doc fix in coefficient outputs, to match format by GLM v1.3.10
@crsl4
Copy link
Member Author

crsl4 commented Sep 5, 2020

ok! to make sure I am on the same page: we need a test for the new writeTopologyLevel1 or a test that covers the new lines added at the end of snaq (or both)?

@cecileane
Copy link
Member

both ideally.

I see that the tests are broken with julia v1.5, see starting here (including a conflict between our own rotate! and LinearAlgebra.rotate!. Should we re-name to rotate edges!?). Some of the tests that failed with bootsnaq are concerning too.

limit what LinearAlgebra is brough into scope, because of its rotate! in v1.5
fixed tests based on RNG
@cecileane
Copy link
Member

ahh.. the tests that were affected by the change in the RNG pass on julia v1.5 but break on julia v1.4, or vice versa. So we can't ask Travis to test both under julia v1.4 and v1.5 successfully. I would like to cancel the tests that are currently running (for commit e60f7f6) but I don't have permission.

@crsl4
Copy link
Member Author

crsl4 commented Sep 6, 2020

sorry that you do not have permission to cancel them! I think we tried before to give you these permissions, but I cannot find the option in travis.

sorry also that I am getting a bit lost (and slow) in this PR! I will pull your changes, and add the new tests. Please let me know if I am missing something

@crsl4
Copy link
Member Author

crsl4 commented Sep 6, 2020

@cecileane how do I know if the coverage increased with these two new commits? I added the tests and tested locally on julia 1.5. But I do not know how to check the coverage.

@cecileane
Copy link
Member

After all the tests are done, you can go over to coveralls for PhyloNetworks, say, then pick a commit that was tested, follow the link to know more about coverage and coverage change due to this commit like here, then click on any file (like readwrite)to see the changes to this file, and what's covered or not.

Codecov has another report, which I find a little easier to use. Like here for the current pull request. (click on Files then browse to see coverage for each file).

Copy link
Member

@cecileane cecileane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thank you @crsl4!

@cecileane cecileane merged commit 7f122fe into master Sep 7, 2020
@cecileane cecileane deleted the minor-fixes branch September 7, 2020 18:37
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.

None yet

3 participants