RacimoLab / demes-r

R library for parsing Demes demographic models
Other
0 stars 2 forks source link

decide on the API for the library #2

Closed grahamgower closed 1 year ago

grahamgower commented 1 year ago

There probably only need to be a small number of functions, probably 2 for loading and 2 for saving. The functions could be named like the demes-python library (demes.load(), demes.loads(), demes.dump(), demes.dumps()), or perhaps name them to match an R library like yaml would make more sense (demes.load(), demes.load_file(), as.yaml(), write.yaml()).

gregorgorjanc commented 1 year ago

I think that matching Python would make most sense and would ease maintenance and porting functionality (without the need to translate function names).

reedacartwright commented 1 year ago

Any thoughts on adding visualization functions to the library?

reedacartwright commented 1 year ago

I recommend using underscores instead of dots in the function names.

grahamgower commented 1 year ago

Any thoughts on adding visualization functions to the library?

I think visual feedback when writing models is really important for users to get immediate feedback about whether their model is what they think it is. Many simple errors can be caught and fixed very quickly, simply by incorporating this into the model-writing workflow. Demesdraw was specifically developed for this purpose (as well as an aid to Demes-related documentation). While it's written in Python, it does have a command line interface, so R users are not excluded from using it simply because it's written in Python. That said, if R users actively avoid demesdraw just because they can't use it from within R (which would make me sad), we might consider providing something that is R specific. If folks have other problems with demesdraw (it's definitely not perfect), I'd be really interested in getting critical feedback so that it can be improved.

reedacartwright commented 1 year ago

I think adding an R-native version of demesdraw—maybe as a ggplot2 layer—would be a fruitful exercise. Working with demes data to build a visualization would demonstrate how R users would interact with demes data and help define how demes should be formatted for R users.

IsabelMarleen commented 1 year ago

I think calling the functions demes_load() is not a native R way to name functions. While there is no universal R naming guide, it seems to me that the tidy verse conventions are used in many packages. The general principle is to use verbs for function names and nouns for variable names, e.g. load_demes().

It would deviate from the API in the python parser, but given the small number of functions I don't expect it to be an issue.

grahamgower commented 1 year ago

Just so that everyone is on the same page, we want to define 4 different functions (as a first step), and I think it is important that there is at least consistency among these functions within the demes-r library.

I think calling the functions demes_load() is not a native R way to name functions. While there is no universal R naming guide, it seems to me that the tidy verse conventions are used in many packages. The general principle is to use verbs for function names and nouns for variable names, e.g. load_demes().

Using verbs for the function names is a fine convention. I think that demes-python does this too. The main difference is that demes-python advocates an explicit demes namespace. E.g., a user is expected to import demes and then use the function demes.load(). I see that R does also support namespaces, using the double colon operator (::) to specify the library to which a function belongs, although doing library(foo) will put foo's functions at the front of the search path (unlike in Python). I think this means that having a load() function in demes-r would mean that one could call demes::load(), but attaching the library with library(demes) would have the unfortunate side effect of overriding the load function in base R. Not good.

It seems we're left with two options: either (1) introduce our own prefix to serve the purpose of a namespace-by-convention, similar to what folks do in C libraries (e.g. demes_load(), demes_dump(), etc.); or (2) we disregard namespaces altogether. My preference is for (1), because I think that (2) leads to user code that is more difficult for others to read.

Ping @bodkan and @molpopgen

It would deviate from the API in the python parser, but given the small number of functions I don't expect it to be an issue.

I agree that deviating from demes-python names for 4 functions should be fine.

gregorgorjanc commented 1 year ago

read_demes() and write_demes() would be what an R user would expect, in my view.

so then one can do

library(demes-r)
my_model <- read_demes(bla bla)
my_model <- rdemes::read_demes(bla bla)
molpopgen commented 1 year ago

I have ~zero at stake here, but it is a bad idea in R to clash with base function names unless you are completely replacing their functionality (like what dplyr does, for example). It is also uncommon to refer to packages by library::function notation. So it seems like prefixes is the best option.

reedacartwright commented 1 year ago

read_demes() and write_demes() would be what an R user would expect, in my view.

I agree. read_demes() and write_demes() would fit in with best practices for function naming in R.

IsabelMarleen commented 1 year ago

read_demes() and write_demes() would be what an R user would expect, in my view.

I also agree. Since keeping the Python function names would clash with base function names or introduce prefixes that are unintuitive to R users, we might as well name the functions in an R native way. There are so few functions that it should be manageable for both users and maintainers.

I don't see the lack of enforced namespace as an issue, because R does not use namespaces that way and R users don't expect it to. Forcing namespaces might be more confusing than anything.

I feel like everyone has shared their view and there seems to be some consensus, so I'll close this issue now.