search

gaynorr / AlphaSimR

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

Inconsistent behaviour when merging pop and a NULL pop #190

Closed gregorgorjanc closed 2 weeks ago

gregorgorjanc commented 2 weeks ago

Describe the bug I think I have found an inconsistent behaviour when merging pop and a NULL pop - one of the annoying edge cases.

Steps To Reproduce

library(AlphaSimR)
founderPop = quickHaplo(nInd=2, nChr=1, segSites=10)
SP = SimParam$new(founderPop)
pop = newPop(founderPop, simParam=SP)
pop2 = pop
is(c(pop, pop2)) # "Pop" "RawPop"
is(c(NULL, pop)) # "list" "vector"
is(c(pop, NULL)) # "Pop" "RawPop"
is(c(NULL, NULL)) # "NULL" "OptionalFunction" "optionalMethod"

is(mergePops(list(pop, pop2))) # "Pop" "RawPop"
is(mergePops(list(pop, NULL))) # "Pop" "RawPop"
is(mergePops(list(NULL, pop))) # "Pop" "RawPop"
is(mergePops(list(NULL, NULL))) # Error in popList[[1]] : subscript out of bounds

is(mergePops(c(pop, pop2))) #  Error in as.list.default(X) : no method for coercing this S4 class to a vector
is(mergePops(c(pop, NULL))) #  Error in as.list.default(X) : no method for coercing this S4 class to a vector
is(mergePops(c(NULL, pop))) # "Pop" "RawPop"
is(mergePops(c(NULL, NULL))) # Error in mergePops(c(NULL, NULL)) :trying to get slot "nLoci" from an object of a basic class ("NULL") with no slots

Expected behavior This has recently come up in simulating a transition from one breeding programme to another. I would have liked that is(c(NULL, pop)) # "list" "vector" and is(c(pop, NULL)) # "Pop" "RawPop" both return a class of object Pop. But maybe this is unavoidable? Any suggestions on how to handle this better? Happy to look into code fix if needed, but would be good to understand what the behaviour should be before we jump into fixing it.

gregorgorjanc commented 2 weeks ago

I have now written my script code such that pop = c(pop, NULL) so I can avoid the above behaviour of c(NULL, pop). Will leave this issue open for now so we can discuss if we need to do anything ...

gaynorr commented 2 weeks ago

@gregorgorjanc I remember adding rules for NULL in c() with populations, but I can't remember the code that motivated me to do this. Do you have an example script showing where the NULL comes from?

The S4 dispatch uses the class of the first object, so this is why it works with a Pop class first but not NULL first. We could potentially use setClassUnion, but this might just end up encouraging bad coding practice. It is probably better practice to avoid having NULL and instead have empty populations (e.g. use of newEmptyPop()). Is there a place in AlphaSimR where NULL is being returned that could be changed to create an empty population?

gregorgorjanc commented 2 weeks ago

The above code shows the behaviour in a comprehensive way, but after I posted this issue I have been thinking along your response - that the code does the right thing by following the S4 rules to dispatch a method based on the first object class. OK, all is well then.

I have NULL populations (because we did not had newEmptyPop() before) and scripts with tests like if (!is.null(pop)), but this can now be easily changed to using newEmptyPop() and tests like if (!nInd(pop) == 0). I think this is indeed better/cleaner coding practice!

I think we can close this issue.