strataG icon indicating copy to clipboard operation
strataG copied to clipboard

Duplicate names

Open konopinski opened this issue 4 years ago • 6 comments

Dear Eric, I found a small problem in .fscCollectParams. I wanted to keep stable growth in a population that is a sink for another population. The thing is I wanted fastsimcoal to estimate this parameter while using the same parameter name between blocks is not allowed by .fscCollectParams. The error is as follows: Error in .fscCollectParams(p) : Can't have duplicated parameter names. I have tested it on the tpl file in fastsimcoal and the program works and substitutes the value with the same random number in par file. Of course one can define another growth parameter but as you know, overparametrisation is an enemy of decent modeling. Perhaps, you could change the check to allow for the same names used for the same parameters, I guess it applies only to growth that can be defined in demes and perhaps a time of an event which could be the same as a time of deme sampling, or migration if it is previously named in the migration matrix (gives the same error message; although, I cannot imagine any situation where this functionality could be useful ;).

In my case it is: demes <- fscSettingsDemes(fscDeme("NFL",15,0,0,"GFL"), fscDeme("ND34",15,0,0,"GD34"),#3 fscDeme("NPL",15,0,0, "GPL"), #4 and events <- fscSettingsEvents( fscEvent("TPL", 2, 1, 1, 1, "GD34", 0), ... where "GD34" is a problem. By the way, I think there is a mistake in the manual for the fastsimcoal use in the strataG. It says in the 'events' section: "growth.rate gives a new growth rate for the source deme" while fsc manual says it is a sink deme that is affected.

konopinski avatar Apr 16 '21 00:04 konopinski

Thanks for raising this. I hadn't considered this use when I put the check for duplicate names in. To make it intelligent as you suggest (where it allows the same name for the same parameter) would require a bit more thought and focus than I can give at this moment.

Would you mind being a beta tester? I'll remove that check altogether and you can let me know if it causes any unforeseen problems. Once I get some time to deal with a few other issues before a CRAN release, I'll try to make this check smarter.

Cheers, Eric

EricArcher avatar Apr 16 '21 04:04 EricArcher

I'm in.

konopinski avatar Apr 16 '21 14:04 konopinski

OK. I've commented it out in the latest push. Let me know if it has any funky downstream effects. I'll get to making it smarter when I have some time to focus on it and will leave this Issue open as a reminder.

EricArcher avatar Apr 16 '21 16:04 EricArcher

I've created a rough solution on a separate branch (duplicate_issue) with my commit. Would you allow access for 'konopinski'? Or it is not the way it works. If you want to include it yourself, the fragment is as follows:

times <- unique(params[grep("time", params[, 3]), 1])
   migrations <- unique(c(params[grep("migrants", params[, 3]), 1], 
                   param)[params[, 2] == "migration", 1])
   growths <- unique(params[grep("growth", params[, 3]), 1])
   genetic <- params[params[, 2]=="genetics", 1]
   if (sum(length(times), length(migrations), length(growths), length(genetic)) > 0  & 
           any(c(match(times, migrations), match(times, growths), match(migrations, growths),
             match(genetic,unique(c(times,growths,migrations)))),na.rm = TRUE)==TRUE){
     stop("Can't have same names for different parameter kinds (e.g. time & growth).")
   }

Sorry for the 'shape' - I'm self-learner and I never learned to write my script more clearly. But it works ;) Cheers, Maciek

konopinski avatar Apr 17 '21 11:04 konopinski

Dear Eric, I found a workaround for this problem. One may define complex parameter like this: fscEstParam("GD1", is.int = FALSE, value = "GD0 + 0", output=FALSE) It works. However, the R solution works too.

konopinski avatar Apr 19 '21 03:04 konopinski

Thanks for your code. I've copied it into my devel version and will test it out when I get back to finishing the package cleanup and CRAN resubmission (hopefully this summer). Good that you found a workaround that'll hold you until then.

Cheers, Eric

EricArcher avatar Apr 19 '21 17:04 EricArcher