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

220 add bruvos between #223

Merged
merged 3 commits into from
Jun 29, 2020
Merged

Conversation

davefol
Copy link
Contributor

@davefol davefol commented Jun 22, 2020

Closes #220

bruvo.between() exposes the same api as bruvo.dist() except for the extra required argument ref.
It also returns the same object except for NaN for distances that are not a comparison between query and ref.

Internally it uses bruvo_between() which skips unneeded values and fills them with 100, later converted to NaN.

@zkamvar zkamvar self-requested a review June 23, 2020 02:55
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

That was quick and thorough! I'm a bit surprised you used the micro-update feature (I haven't used that myself in quite a while). Your approach to not re-invent the wheel is fantastic and will make this code easier to maintain in the long run. I admit that I wasn't quite sure what I was expecting with this approach, but now that I see it, it makes a lot of sense.

Thank you for the effort 💯

I do see a couple of things that can be improved:

  1. Since you're adding a new function, you should change your role as "aut" in the DESCRIPTION
  2. Please add an @export tag in the bruvo.between documentation
  3. The matrix got flipped somewhere in the C implementation, because I'm getting the following:
library("poppr")
#> Loading required package: adegenet
#> Loading required package: ade4
#> Registered S3 method overwritten by 'spdep':
#>   method   from
#>   plot.mst ape
#> 
#>    /// adegenet 2.1.3 is loaded ////////////
#> 
#>    > overview: '?adegenet'
#>    > tutorials/doc/questions: 'adegenetWeb()' 
#>    > bug reports/feature requests: adegenetIssues()
#> Registered S3 method overwritten by 'pegas':
#>   method      from
#>   print.amova ade4
#> This is poppr version 2.8.6.99.8. To get started, type package?poppr
#> OMP parallel support: available
#> 
#> This version of poppr is under development.
#> If you find any bugs, please report them at https://github.com/grunwaldlab/poppr/issues
data(nancycats)
n3 <- nancycats[pop = 3]
n4 <- nancycats[pop = 4]
btwn <- poppr:::bruvo.between(n3[1:3], n4[1:5], replen = rep(2, 9), by_locus = TRUE)
btwn
#> $fca8
#>           N24       N25       N26       N35       N36       N37       N38
#> N25        NA                                                            
#> N26        NA        NA                                                  
#> N35 0.7500000 0.7500000 0.7500000                                        
#> N36 0.3750000 0.3750000 0.3750000        NA                              
#> N37 0.4921875 0.4921875 0.4921875        NA 0.2500000                    
#> N38 0.4921875 0.4921875 0.4921875 0.8437500 0.4687500        NA          
#> N39 0.0000000 0.0000000 0.0000000 0.4687500 0.7480469 0.8437500 0.4687500
#> 
#> $fca23
#>           N24       N25       N26       N35       N36       N37       N38
#> N25 0.2500000                                                            
#> N26 0.4687500 0.7187500                                                  
#> N35 0.7480469        NA 0.3750000                                        
#> N36 0.3750000        NA 0.3750000 0.3750000                              
#> N37 0.0000000 0.7500000        NA 0.0000000 0.8437500                    
#> N38 0.4375000 0.7500000 0.3750000 0.0000000 0.3750000 0.0000000          
#> N39 0.0000000 0.0000000 0.8437500 0.3750000 0.0000000        NA        NA
#> 
#> $fca43
#>           N24       N25       N26       N35       N36       N37       N38
#> N25 0.4921875                                                            
#> N26 0.4921875 0.4375000                                                  
#> N35 0.4843750 0.3750000 0.4375000                                        
#> N36 0.4843750 0.3750000 0.4375000 0.9375000                              
#> N37 0.4375000 0.0000000 0.4843750 0.9062500 0.9843750                    
#> N38        NA 0.3750000        NA 0.8750000        NA 0.8125000          
#> N39 0.4375000 0.3750000        NA 0.9843750 0.8437500 0.8750000 0.4921875
#> 
#> $fca45
#>           N24       N25       N26       N35       N36       N37       N38
#> N25 0.4921875                                                            
#> N26 0.8437500        NA                                                  
#> N35 0.8125000 0.0000000        NA                                        
#> N36 0.8750000 0.4375000 0.2500000 0.4687500                              
#> N37 0.4921875 0.4375000 0.4687500 0.7187500 0.3750000                    
#> N38 0.4921875 0.6250000 0.4687500 0.9375000 0.9375000        NA          
#> N39        NA 0.4375000 0.3750000 0.9375000        NA 0.8437500 0.8437500
#> 
#> $fca77
#>           N24       N25       N26       N35       N36       N37       N38
#> N25 0.8437500                                                            
#> N26 0.0000000 0.7500000                                                  
#> N35 0.4687500 0.7500000 0.4687500                                        
#> N36        NA 0.8437500        NA 0.8745117                              
#> N37 0.4921875 0.8437500        NA 0.8745117 0.4980469                    
#> N38 0.4921875 0.8437500 0.4980469 0.0000000 0.4995117 0.8745117          
#> N39 0.4921875 0.0000000 0.4995117        NA 0.8745117 0.0000000 0.4980469
#> 
#> $fca78
#>           N24       N25       N26       N35       N36       N37       N38
#> N25 0.4995117                                                            
#> N26 0.8745117 0.7187500                                                  
#> N35 0.8745117 0.0000000 0.7187500                                        
#> N36 0.0000000 0.4921875 0.0000000 0.4375000                              
#> N37        NA 0.0000000 0.4921875 0.5000000        NA                    
#> N38        NA        NA 0.0000000 0.7460938        NA        NA          
#> N39 0.4375000 0.4375000 0.7187500 0.5000000        NA        NA        NA
#> 
#> $fca90
#>     N24 N25 N26 N35 N36 N37 N38
#> N25  NA                        
#> N26  NA  NA                    
#> N35  NA  NA  NA                
#> N36  NA  NA  NA  NA            
#> N37  NA  NA  NA  NA  NA        
#> N38  NA  NA  NA  NA  NA  NA    
#> N39  NA  NA  NA  NA  NA  NA  NA
#> 
#> $fca96
#>     N24 N25 N26 N35 N36 N37 N38
#> N25  NA                        
#> N26  NA  NA                    
#> N35  NA  NA  NA                
#> N36  NA  NA  NA  NA            
#> N37  NA  NA  NA  NA  NA        
#> N38  NA  NA  NA  NA  NA  NA    
#> N39  NA  NA  NA  NA  NA  NA  NA
#> 
#> $fca37
#>     N24 N25 N26 N35 N36 N37 N38
#> N25  NA                        
#> N26  NA  NA                    
#> N35  NA  NA  NA                
#> N36  NA  NA  NA  NA            
#> N37  NA  NA  NA  NA  NA        
#> N38  NA  NA  NA  NA  NA  NA    
#> N39  NA  NA  NA  NA  NA  NA  NA
# 3 * 5 * 9
sum(!is.na(unlist(btwn)))
#> [1] 135

Created on 2020-06-22 by the reprex package (v0.3.0)

The numbers are correct, they are just in the wrong order. It would be good to add a test for that.

That being said, I can take care of these on the weekend if you don't have time to get to these this week.

@davefol
Copy link
Contributor Author

davefol commented Jun 23, 2020

Hi Zhian, I think I handled everything in d862c3e

Let me know if I missed anything.

@zkamvar
Copy link
Member

zkamvar commented Jun 29, 2020

It looks good to me! The documentation needed to be re-roxygenized and NEWS updated, but I can take care of that. Thank you for the contribution!

@zkamvar zkamvar merged commit 3706895 into grunwaldlab:main Jun 29, 2020
@davefol davefol deleted the 220-add-bruvos-between branch June 29, 2020 03:00
@davefol
Copy link
Contributor Author

davefol commented Jun 29, 2020

Thanks for making it so easy. Would love to dig into the library more and contribute where needed.

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.

Bruvos Distance function comparing n individuals to a reference set.
2 participants