conventions on variables, arguments, functions, etc

[bnicenboim]

Hi,
As I said before, amazing idea! I'm sorry to be annoying with this (which seems not that important, but in my experience it is). But it would be good to have some wiki/contribution guidelines for how to name arguments, functions, and variables consistently. This will help (me/others) to contribute new models and to start using the existing functions without the worry that they will change (a lot, of course they'll change it's in development):

I've noticed that there are already some inconsistencies, so for example here:

IMMfull(resp_err = "dev_rad", nt_features = paste0("col_nt", 1:7), nt_distances = paste0("dist_nt",1:7), setsize = "set_size")

Shouldn't it be set_size? Does it make sense to cut resp_error into resp_err? Also, how are models named? Given that you have IMMfull (why not IMM_full?), shouldn't it be SDMsimple (or better SDM_simple) rather than sdmSimple?

[bnicenboim]

This might be too related to #134. But my main point would be to have that in the contribution guidelines.

[GidonFrischkorn]

Hi Bruno,
thanks for bringing this point up, and yes this is something we should further clarify in the developer notes! There have been some changes that are not yet reflected there, so they need to be updated ASAP anyway. Generally, I agree that we should look over the naming scheme and use snake_case consistently.

So for example setsize should be set_size, comparable to nt_features and I also see that we could change resp_err to resp_error as it does not really lead to a significantly longer variable name.

Regarding the model naming, there is a clear tendency for @venpopov and me to go for an additional version argument inside the model call. From our perspective this would clarify that the same model can be applied in different tasks or flavors depending on the assumptions a researcher wants to make.

Should you have more feedback, feel free to bring it up here. We are happy about everything that helps getting this package to an easy to use and flexible tool that as many people as possible can contribute to.

[venpopov]

I agree. This has bugged me for a while and a few weeks ago I changed all internals to underscore conventions, and was planning to raise an issue for the user facing functions as well.

My preference would be for everything to be using underscores and lower case. sdm_simple, etc. @GidonFrischkorn thoughts?

[venpopov]

Posted this before seeing your comment above. Yes, I'm forr the version argument we discussed, but for everything else use that naming consistency

[venpopov]

I also wanted to comment briefly on the likelihood of changes. As you noted, we are in development, so some things will change, but I think we are at a stage where most of the core functionality of the package is stable. We plan to submit to CRAN in the next few months, so after that stability should be guaranteed and any breaking changes will be introduced gradually with temporary backwards compatibility.

[venpopov]

@GidonFrischkorn related to this, we might want to also rethink the parallel = TRUE argument.

I don't remember why we had it like this, but it's inconsistent with both brms and cmdstanr, which use different arguments each. In brms you specify the cores argument with a number, and in cmdstanr you specify "parallel_chains" - both do the same. I think we should probably just get rid of the parallel argument and users can just use the core argument just like they would in brms

[GidonFrischkorn]

I see your point, we could make this a bmm.option and set the option for mc.cores internally if bmm.option(parallel = True). But I agree that adding this as an option to the fit_model call is not necessary.

[venpopov]

Ok, @GidonFrischkorn here's a proposal for changes, some of which we've discussed over the last months:

• fit_model() -> bmm()
• mixture2p and mixture3p -> mixture_vwm() with a version argument for 2p or 3p
• sdmSimple -> sdm() with a version argument
• IMM* (all versions) -> imm() with a version argument for abc, bsc, full
• dIMM and friends -> dimm
• setsize argument -> set_size
• OberauerLin_2017 -> oberauer_lin_2017
• ZhangLuck_2008 -> zhang_luck_2008
• resp_err -> resp_error (because of R's partial matching of arguments, you can still use resp_err even if it is longer :) )
• deprecate parallel option
• if anything else internal that comes up that's not snake_caes gets that, although I think the things above are the few things we have that are not, almost everything else is already that way. One exception is bmmformula, but I think it makes sense to keep it that way for the similarity with brmsformula

This swtich to the version argument for models instead of separate functions will be a bit more work than just renaming the other arguments, but I'm pretty sure it shouldn't be much of a problem and can get this done we agree this is the way to go.

[bnicenboim]

by the way, you also have bmmmodel, where b.m.m. already includes model ;)

[GidonFrischkorn]

Fair point. When writing some vignettes, I already thought that something is weird in writing bmmmodel and you are totally right it should only be bmm.

@venpopov maybe we can edit the vignettes to say model_name instead of bmmmodel.

[GidonFrischkorn]

Ok, @GidonFrischkorn here's a proposal for changes, some of which we've discussed over the last months:

• fit_model() -> bmm()
• mixture2p and mixture3p -> mixture_vwm() with a version argument for 2p or 3p
• sdmSimple -> sdm() with a version argument
• IMM* (all versions) -> imm() with a version argument for abc, bsc, full
• dIMM and friends -> dimm
• setsize argument -> set_size
• OberauerLin_2017 -> oberauer_lin_2017
• ZhangLuck_2008 -> zhang_luck_2008
• resp_err -> resp_error (because of R's partial matching of arguments, you can still use resp_err even if it is longer :) )
• if anything else internal that comes up that's not snake_caes gets that, although I think the things above are the few things we have that are not, almost everything else is already that way. One exception is bmmformula, but I think it makes sense to keep it that way for the similarity with brmsformula

This swtich to the version argument for models instead of separate functions will be a bit more work than just renaming the other arguments, but I'm pretty sure it shouldn't be much of a problem and can get this done we agree this is the way to go.

As far as I see, these changes tap the most inconsistencies. I was thinking, if we should keep the fit_model function as an alias of bmm to maintain backwards compatibility. We can add a warning that its use is depercated and that users should switch to bmm.

[venpopov]

alright, I'll get started on this

[venpopov]

All naming changes are now implemented in the latests v0.5.1 release. Thanks again @bnicenboim for bringing this up. In the next few days we'll add a section on the developer's guide (https://venpopov.github.io/bmm/dev/dev-notes/) about the naming conventions