search

ropensci / EML

Ecological Metadata Language interface for R: synthesis and integration of heterogenous data
https://docs.ropensci.org/EML
Other
98 stars 33 forks source link

ensure dateTime domain is recognized by `get_attributes` #262

Closed jeanetteclark closed 5 years ago

jeanetteclark commented 5 years ago

the dateTime measurement scale is not required to contain a dateTimeDomain - only a formatString. As a result, get_attributes will populate the domain column with NA if the dateTimeDomain is not given.

This seems like less than ideal behavior to me given that set_attributes will return Error: The domain column must be filled for each attribute. if the dateTimeDomain is not included.

Another option to fix this is to fix set_attributes so that it does not require a domain for dateTime measurement scales...but this seemed like the path of least resistance.

Also I am not sure why my other commits are included in this PR since they got merged a while ago

cboettig commented 5 years ago

@jeanetteclark Thanks! Can you add yourself as contributor (ctb code) to the DESCRIPTION as well?

I think the old commits show up because the commits were squashed in a 'squash-and-merge' when that PR was merged? I'm not sure but shouldn't be an issue.

In other news, it looks like travis/appveyor are failing for unrelated reasons over some units fragility -- we should probably patch that first in any event. Feel free to send a separate PR on that if you already have an idea what's wrong there (we should possibly consider moving these fragile unit manipulations off to a units branch until they are ready for prime-time anyway?) or else I'll try and find some time to patch those up first so we can merge against a clean set of tests...

jeanetteclark commented 5 years ago

Yeah I think the units would be fine on a branch for now if thats the route you want to go - I've had a look at some of it (I think Mitchell did some of the custom units stuff, right?) but haven't had time to wade through it enough to figure out what exactly is going wrong.

cboettig commented 5 years ago

yup, Mitchell contributed pretty much all the development of the custom unit stuff, (both shiny-based and command line), including the pretty extensive test suite that was passing somehow when we merged it, but when I last looked at it I couldn't figure out how it ever worked.

It's kinda a pity that it's not working right now since helping users with units could be really valuable, but clearly needs more development.

jeanetteclark commented 5 years ago

It might actually be a pretty simple fix. In the new EML, eml_units <- get_unitList()$units (line 65 in https://github.com/ropensci/EML/blob/master/R/get_unit_id.R) returns a data.fame but each row is a list of one item. In old EML that returns a more typical looking data.frame

cboettig commented 5 years ago

:+1: cool! I still have to dig myself out of a few other holes before I can jump back into the code here but I'd welcome any PRs to address this! Would be great to get a 2.0 release on it's way to CRAN...

cboettig commented 5 years ago

well that looks like a weird error for travis... Looks to me like the install step can't find memoise but it's on Travis's package list of installed packages already....

jeanetteclark commented 5 years ago

yeah, turns out the code was actually fine.

My new thinking is it is related to either memoise or the units package

jeanetteclark commented 5 years ago

(nothing new in this build...I amended a commit message)

jeanetteclark commented 5 years ago

Finally was able to replicate the failure locally by running devtools::test() as opposed to devtools::check()

Turns out it fails on this line: read_eml(system.file("tests", options("emld_db"), "eml-unitDictionary.xml", package = "emld"))

loading the emld library locally fixes it. It seems strange to me that the same call does not fail in test-units.R. In any case, it seems that Travis can't find this file in some cases?

cboettig commented 5 years ago

Ha, nice work!

Yes, that line is problematic. The part that says options("emld_db") should be replaced with: emld::eml_version()

The line is failing often becuase emld_db is usually not defined (i.e. no default EML version is defined. The new function emld::eml_version() uses that same option under the hood to set the EML version, but has a smart default and is hopefully more meaningful name than emld_db)!!

jeanetteclark commented 5 years ago

Ahh okay! Alright lets see if this works. Good results locally

jeanetteclark commented 5 years ago

I'm sure you've figured this out already, but the call mem_load_udunits <- memoise::memoise(load_udunits) (which occurs outside of a function) is what is causing the build to fail using devel. I talked with @amoeba about this a bit today and investigated a couple of options that allow us to get rid of memoise entirely

Happy to hear your thoughts. This pull request has really taken quite the turn

cboettig commented 5 years ago

Cool, that makes sense. I think sysdata.rda is probably the way to go here. As you've probably seen, we already use sysdata for the standardUnits list, so may as well do it for the udunits as well, particularly as the size increase is quite small.

Nice work on this, this certainly has taken a turn!

cboettig commented 5 years ago

Yay, 🎉 this is great in so many ways! Nice to slim the dependencies a bit, improve the stability of our units unit-tests, and update the versioning referencing while also fixing the dateTime domain stuff. great work. :shipit:

jeanetteclark commented 5 years ago

whoop! while I'm in here I also have a fix for issue #260 coming shortly...will submit as another PR