ocean-tracking-network / glatos

9 stars 4 forks source link

`fasttime` dependency/documentation is unneeded #211

Closed mhpob closed 3 months ago

mhpob commented 10 months ago

In the interest of one less dependency, fasttime was functionally baked into lubridate in 2013 and lubridate doesn't seem to have recognized option(lubridate.fasttime = TRUE) since around that time, as well.

Fix is simple: remove the lines below and drop fasttime from DESCRIPTION. Unfortunately, there's a lot of documentation that references fasttime::fastPOSIXct that would require a bit more work.

Benchmarks of the respective unit tests with and without the noted lines give the same result:

read_otn_detections

Unit: milliseconds
         expr     min      lq     mean   median      uq     max neval
     fasttime 16.1708 16.5407 17.51958 16.75265 17.6666 32.2774   100
  no_fasttime 16.0693 16.4321 17.58034 16.79735 17.4881 37.6090   100

read_glatos_receivers

Unit: milliseconds
         expr     min      lq     mean   median      uq     max neval
     fasttime 11.4339 11.7742 12.98503 12.10125 12.6295 34.0413   100
 no_fastttime 11.3979 11.8468 12.80299 12.14700 12.5707 36.3602   100

read_glatos_detections

Unit: milliseconds
        expr     min       lq     mean   median       uq     max neval
    fasttime 42.3853 43.17120 47.45281 43.92875 50.82940 96.2659   100
 no_fasttime 42.5226 43.14825 47.44075 44.38395 51.65755 75.0236   100

read_otn_deployments untested due to #202

chrisholbrook commented 10 months ago

I support this change. FWIW, I usually go right to lubridate::fast_strptime(); first version of read_glatos_detections() would have likely used fasttime::fastPOSIXct directly (hence why that's prevalent in docs); but I'm not sure why we went to parse_date_time here instead of fast_strptime().