Adding Achilles to HADES

[schuemie]

The Achilles README file currently states that Achilles is part of HADES, but this is not (yet) the case. There are several important requirements that need to before we can add Achilles to HADES (which I would very much like to see):

1. HADES packages can only depend on packages in CRAN, and on other HADES packages. That is because both have clearly defined release conventions, so we can always get the lastest version, or some specified historic version. Achilles depends on Castor, which is in neither CRAN nor HADES. Either move the code required by Achilles into Achilles (my preference), or go through the process of adding Castor to HADES.
2. The head of the master branch should be the latest released version, so remotes::install_github() installs the latest released version, not the latest develop version as is currently the case.
3. GitHub actions should be in place to run R check, and automatically create releases when there's a push to master. (see other HADES packages).
4. Unit tests should be added / restored (there used to be unit tests against the OHDSI database testing servers)
5. The documentation website should adhere to the HADES style (see other HADES repos).

[schuemie]

Is there no interest in moving Achilles into HADES? Or at least have releases? Wouldn't continuous integration be nice?

[fdefalco]

Both are of interest and if you review, you'll notice that the developers have actively been reducing the gap between Achilles and Hades eligibility.

[schuemie]

Hi @fdefalco , I reviewed the current version of ACHILLES in the develop branch. Here are my findings:

• DESCRIPTION: Please remove Remotes section with reference to Castor
• DESCRIPTION: Please use a 3-digit version number
• DESCRIPTION: Don't forget to update Date
• DESCRIPTION: Recommend changing Title field to 'Automated Characterization of Health Information at Large-scale Longitudinal Evidence Systems'
• Update copyright year in all files (using OhdsiRTools::updateCopyrightYearFolder())
• R check should have no Warnings (and ideally no Notes, unless there's a good reason)
• DESCRIPTION: add "withr" to Suggests (as indicated by R check Warning)
• Add "#' @import dplyr" to Achilles-package.R (as indicated by R check Note)
• Add "docs" and "extras" to 'Rbuildignore' (as indicated by R check Note)
• Do not import lubridate (to avoid function name collision with DatabaseConnector in R check Warning) You don't have to import lubridate to use lubridate. See https://github.com/OHDSI/Achilles/blob/develop/R/exportAO.R#L1190
• Remove all references to Castor (as indicated by R check Warning). See https://github.com/OHDSI/Achilles/blob/develop/R/performTemporalCharacterization.r#L132-L135
• Are both rjson and jsonlite required? In HADES, people then to use jsonlite or JSONIO, arguing for jsonlite in ACHILLES
• R_CMD_check_Hades.yaml is currently missing code coverage and release actions. Please see other HADES packages (will also need to add "compare_versions" and "deploy.sh" scripts to repo root and .Rbuildignore
• "exportAO" does not use HADES camel case (Should be "exportAo"). Also, why abbreviate Ares? Recommend renaming to "exportToAres" or something similar.
• Be kind to users of database platforms that don't support temp tables (their lives are already hard enough). Delete any temp tables you create. Currently not deleting "#conoc", "#death", "#drexp", "#dvexp", "#msmt", "#obs", and "#prococ".

Most of these changes should be fairly quick fixes, except maybe the rjson dependency.

[schuemie]

Oh, also noticed this one:

• Recommend moving on.exit clause up to right after connecting: https://github.com/OHDSI/Achilles/blob/develop/R/Achilles.R#L1089

[schuemie]

I also see hundreds of tmpach tables. These should be deleted as well, when Achilles is done, right?

[AnthonyMolinaro]

@schuemie Thanks Martijn, good catch!

@fdefalco I'll create a branch to test this on.

[fdefalco]

I merged the fix for this from the PR.