Upon executing the following code a parsing failure is produced:

db <- ukbschemas_db(path = tempdir(), overwrite = T)

The parsing failure reads:

Warning: 1 parsing failure.
row col  expected    actual                                                            file
 84  -- 6 columns 9 columns 'http://biobank.ndph.ox.ac.uk/showcase/scdown.cgi?fmt=txt&id=4'

This is a list of current issues with the README. They can all be fixed together, and this issue should be closed when the list is completed.

• Description isn't very accurate ("create ... the UK Biobank Data Showcase schemas"?)
• The primary (ukbschemas_db()) workflow should be in one block
• Low-level headings should separate the two workflow options

When loading tidied tables, the tables are not stored in the form pre-specified by the CREATE TABLE SQL statements. Variable order and type are incorrect. For example:

db <- ukbschema::create_schema_db(path = tempdir())

The table encvalues is expected to have structure:

CREATE TABLE encvalues(
  "encoding_id" INTEGER,
  "code_id" INTEGER,
  "parent_id" INTEGER,
  "type" TEXT,
  "value" TEXT,
  "meaning" TEXT,
  "selectable" INTEGER,
  "showcase_order" INTEGER,
  PRIMARY KEY ("encoding_id", "code_id")
);

But then

$ sqlite3 ukb-schema-2019-07-11.sqlite ".schema encvalues"
CREATE TABLE `encvalues` (
  `encoding_id` REAL,
  `value` TEXT,
  `meaning` TEXT,
  `showcase_order` REAL,
  `parent_id` INTEGER,
  `selectable` INTEGER,
  `type` TEXT,
  `code_id` INTEGER
);

When running db <- ukbschemas_db(), either with new temp path or referring to the sqlite file, it gives me the following cryptic error:

Error in guess_header_(datasource, tokenizer, locale) : 
  Expected single logical value

It used to work fine. Did any of the dependencies change?

A current limitation of testing the package is that it needs to download the schema files. Locally, this is handled by keeping a cached copy (and Travis should do the same). However, this is increasingly undesirable as the collective size of the schemas (now >60MB in the sqlite file).

Moreover, any problems with the data which cause parse errors will cause tests and builds to fail until the schema files are corrected by UKB. Responsive as they are, it would be better not to have this point of failure which is out of our control.

It is worth exploring whether a dummy dataset can be created for testing the package (on Travis, especially) which avoids the substantial download of the schema files.

Most of the package works just fine with missing or extra tables because the functions generically save or load whatever tables are present.

.tidy_schemas(), however, fixes up specific tables and needs error handling or warning/skipping missing tables which currently it does not have.

Previously, data.table::fread() (used by the .tryRead() internal function of .import_schemas()) would import date variables as class character, necessitating a subsequent coercion to Date class. In order to achieve this, two further internal functions were used: .isISOdate() & .autoISOdate(). It would appear as though these functions are now redundant. However, as data.table::fread() imports date variables as IDate class, a new step is required to coerce these variables back to Date class. The version of data.table in which this change occurred should also be set as a hard dependency in the DESCRIPTION file.

None of the API functions currently have examples; all should do so.

Also add a test file to ensure the examples run without error.

While it may be appropriate to import 64-bit integers as class integer64 this poses some problems. This first is that it adds an optional dependency for the bit64 package. The second is that it breaks consistency with the fallback readr::read_delim() function of .tryRead(), which does not have the ability to import variables as this class.

Setting the integer64 parameter of fread() to FALSE should rectify this issue, and ensure that all 64-bit integers as imported as class double.

The recommended table seems to link categories and fields in a way that is (presumably) distinct from the category_id column of fields. From the schema table:

UK Biobank maintains lists of 'recommended' data fields which researchers are encouraged to use as starting points for baskets in various areas of interest. Properties included are: Category ID; Field ID. These categories are identified in schema 3 as having group_type=0.

The structure of recommended is not yet clear. If recommended is 1:m or m:1 (categories:fields) then it could be added to the table for which it's entries are NOT unique. A 1:m structure seems likely because categories with recommended fields are flagged in the categories table with group_type == 0.

If 1:1 then it depends on whether category_id in recommended matches category_id in fields for the same field_id. If so, then it can be removed and a recommended flag added to fields. If not, then the category_id could be added to fields as recommended_category_id or similar.

If m:n then it should be left as-is, because it cannot be added directly to either table without row or column duplication.

The package could be migrated to use GitHub Actions for CI.

This will make issue #5 redundant.

Several field properties:

• stability
• item_type
• strata
• sexed

are encoded as integers but have specific meanings which can be gleaned by joining the data dictionary at https://biobank.ctsu.ox.ac.uk/~bbdatan/Data_Dictionary_Showcase.csv with the fields table to identify the code mappings.

Add tables to the database with appropriate data included as internal variables.

Although no one should be referring to the list elements by position, for consistency it would be better for the order to be the same (alphabetical is simplest).

Probably the fix is to add readr.show_progress = FALSE to the temporary option changes in .get_schemas() ( in R/misc-functions.R).

Although the plural of schema is schemata, no one uses that. So schemas it is. And especially, schema is singular.

Usage is currently inconsistent so this should be fixed.

This is a hang-over from the previous bash code, but the R package adds this in .tidy_schemas() instead.

On Travis-CI, build 6 failed because the test suite failed with the following report:

  ── 1. Failure: create_schema_db() fails to overwrite when db is connected (@test
  `{ ... }` did not throw an error.
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  OK: 19 SKIPPED: 1 WARNINGS: 1 FAILED: 1
  1. Failure: create_schema_db() fails to overwrite when db is connected (@test-create-schema-db.R#73) 

The failing test passes on my local Win10 OS, so maybe this is an OS issue?

The current work-around is to skip the test except on Windows, but it would be nice to know whether this goes deeper, e.g. if on Linux systems file.remove() works even when there is an open database connection. If so, some more robust way to check for open connections would be nice to have.

Category 119 is probably best left out of the schemas. It isn't linked to any other category so there is no loss.

UKB may end up fixing category 100032 description instead, and if they decide otherwise then it would be included automatically by create_schema_db() anyway.

E.g. code to preserve and roll-back when writing to database from the r-dbi/RSQLite/R/table.R code:

dbBegin(conn, name = "dbWriteTable")
on.exit(dbRollback(conn, name = "dbWriteTable"))
# ...
dbCommit(conn, name = "dbWriteTable")
on.exit(NULL)

For each function taking this approach, need to add tests to ensure the connection is closed in each circumstance.

In a sense this is the main purpose of the package: to get the schemas into R. But of course it is inefficient to download them every time from the UKB server.

I'm thinking about having a function, probably called ukb_schemas() or ukbschemas(), to do this, but not sure that it is a good idea.

A function like this could help make the create_schema_db() process more modular and user-accessible. If added, it should probably include warnings/recommendations to consider saving the database.

The use of lsof in #22 will allow testing of code to protect open database files, and error handling is implicit.

However, some errors (especially if lsof is missing/unavailable) will be misreported and better handling might be useful.

This code needs review and consideration on how to best implement this fix to #5.