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

Only use names and function signatures that are also valid in Python #47

Closed
2 tasks done
lloyddewit opened this issue Feb 11, 2022 · 8 comments
Closed
2 tasks done

Comments

@lloyddewit
Copy link
Collaborator

lloyddewit commented Feb 11, 2022

The Python layer uses the 'rpy2' Python package. This package allows Python code to refer directly to R functions, parameters and other objects. In order to make the Python code simple and readable, we should minimise the transformations needed between the Python and R layers.

However, R allows some things that are illegal in Python. For example, the climatic_summary() function uses the following practices that are illegal in Python:

  • Some function parameters have a dot ('.') as part of their names. For example, na.rm and summaries.params. To be consistent with Python, they should be named na_rm and summaries_params (or something similar).
  • Some function parameters without default values are listed after parameters with default values. For example, the elements and summaries parameters are in the middle of the list, they should be before the first parameter with a default value.

The Python rpy2 package provides workarounds for the above but they add complexity and reduce readability.

Please would it be possible to avoid the above and only use names and function signatures that are also valid in Python?

Note: If I find new inconsistencies while I implement the Python layer, then I will add extra check boxes to the list above.

@lloyddewit lloyddewit changed the title Dot notation for parameter names, and other public identifiers, causes complications for rpy2 Only use names and function signatures that are also valid in Python Feb 11, 2022
@dannyparsons
Copy link
Contributor

Yes we can do that in the package. Note that these things are common in other R packages, so it may be difficult to avoid this in general in the Python layer when other packages are used.

@lilyclements Could you help to make these changes?

@lilyclements
Copy link
Collaborator

Happy to. Two questions:

  1. Should we look at the functions from R-Instat (or where should I report changes)? I don't want to create any issues down the line
  2. What should we do about na.rm? I assume we should just keep it

@lilyclements
Copy link
Collaborator

I'll also add in checkmate:: where appropriate while I'm at it.

@dannyparsons
Copy link
Contributor

I think just start with the Climsoft-like functions initially.

If we have an na.rm parameter in any of our functions we can change this to na_rm.

@lilyclements
Copy link
Collaborator

Great have done for the climsoft-like functions in #49

In addition, is it okay for the resulting data frame to have variables that contain a .. This can occur in inventory_table and inventory_plot.
(And wwr_export from the non-Climsoftesque functions).

R-Instat functions to edit:
climatic_missing
climdex
climdex_single_station
output_CDT

@dannyparsons
Copy link
Contributor

I think let's try to avoid that too, unless it's a requirement of the output in some way.

@lilyclements
Copy link
Collaborator

Great, Ive done that in PR #49

@lloyddewit
Copy link
Collaborator Author

Fixed in PR #49
Thank you @lilyclements !

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

3 participants