search

gaynorr / AlphaSimR

R package for breeding program simulations
https://gaynorr.github.io/AlphaSimR/
Other
40 stars 17 forks source link

Flipping misc organisation to address #107 #168

Closed gregorgorjanc closed 7 months ago

gregorgorjanc commented 7 months ago

Here is an implementation where we flip misc organisation from ind-node to node-ind for simplicity and speed of operations. As discussed in #107.

Looking forward to feedback;)

gregorgorjanc commented 7 months ago

The last commit is just an exploration of how this new org would work if we use Pop or MultiPop in misc

gaynorr commented 7 months ago

The family pulled me away, so I just pushed something that is incomplete. I tried to add a length method to MultiPop, but it isn't working. Also, the mergePop is failing in the tests inside the testthat tests.

gregorgorjanc commented 7 months ago

Yes, the test is unfinished because I wasn't sure/clear if we want to handle Pop or MultiPop in the new misc organisation. I initiated three cases:

  pop@misc$mtP = popOrig
  pop@misc$mtLP = list(popOrig, popOrig)
  pop@misc$mtMP = multiPop

Note that pop@misc$mtLP (list of Pop) is effectively the same as pop@misc$mtMP (MultiPop) and I think this is how we should move onwards - working with these options.

I was not sure if you wanted to have support for pop@misc$mtP (Pop) as well, but this is a less natural use case (subsetting the misc that contains pop as one of the misc-list-nodes would pick individuals of the population), though we can make it work by developing a length(method) for Pop too (making it the same to nInd()), I think.

gregorgorjanc commented 7 months ago

I have just tested the length() method on MultiPop and it works as expected!

> length(multiPop)
[1] 2
gregorgorjanc commented 7 months ago

This last push makes all the tests run. There is just one test commented out since we cannot create an empty MultiPop. Might add that later in another issue/PR - https://github.com/gaynorr/AlphaSimR/issues/169

gaynorr commented 7 months ago

I think I know how to fix this. It should just be a matter of changing the validity function for MultiPop. It's currently a loop with 1:length(object@pops). This fails when length is 0. I should instead be using seq_along(length(object@pops)), because it works when the value is zero. This is a best practice for R programming that I didn't know about until relatively recently.