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

Duplicate names #52

Open
konopinski opened this issue Apr 16, 2021 · 6 comments
Open

Duplicate names #52

konopinski opened this issue Apr 16, 2021 · 6 comments

Comments

@konopinski
Copy link

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.

@EricArcher
Copy link
Owner

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

@konopinski
Copy link
Author

I'm in.

@EricArcher
Copy link
Owner

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.

@konopinski
Copy link
Author

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
Copy link
Author

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.

@EricArcher
Copy link
Owner

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

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

No branches or pull requests

2 participants