as.ctd(rsk, ...) doesn't pass along metadata fields

[richardsc]

For example:

library(oce)
data(rsk)
print(as.ctd(rsk, ship="Clark's canoe")[['ship']])

[1] "Ault"

print(as.ctd(oceSetMetadata(rsk, 'ship', "Clark's canoe", note='cgr'))[['ship']])

[1] "Clark's canoe"

[dankelley]

In the email thread, @richardsc suggested obeying all the args that relate to metadata, and I'll have a look at that, perhaps before Saturday. (I have a Swiss cheese schedule this week with a lot of unplannable elements.). It makes sense for me to switch it because I know exactly where to look in the code, and I've thought a lot about how to specify arguments.

As for arguments -- the issue is that there can be problems with default values. For example, if there is an arg named foo that has default "bar", then the function will have no way to know whether (a) the user specified "bar" to alter the value that was in the rsk object, or (b) the user didn't specify anything, and will expect that the the value in the rsk object should be used. The only way to handle that kind of thing is to have default-less arguments. But there was a time when I was moving away from default-less arguments, because the Rstudio code checking algorithm flagged such things as problems. (I should have known better -- warning on such things is not a violation of R rules, and it is also silly, in cases like the one in discussion.)

[dankelley]

I just took a look, and as.ctd basically lists most args with default values, so the problem I mention in the previous comment may be pertinent. I would bet some money that I did it this way because of the RStudio diagnostic messages.

I'm a bit reluctant to change all the defaults to be the nondefault scheme, because of code ripples. However, some things should be easy. I suspect (but don't know -- @richardsc, can you comment?) that rsk files can never hold longitude and latitude, and therefore the arg values (which are NA) should just get copied in. Similar for ship, I imagine. Here is the list of what I think we should copy in from the args, and I'm hoping @richardsc will edit this comment to write "CR agrees" or "CR" disagrees" at the end of each item in the list, and also that he will add new items as he feels appropriate. This seems like just 5 minutes of work to make the list, and the coding should only take 1/2 an hour.

I will code anything on which both I and CR say "USE", and click checkboxes when I do that. Coding will include also documenting ... what this means is that any items on which both I and CR say "IGNORE" will simply be covered by a phrase like ("all other arguments are ignored") in the doc. Once CR votes on things, we can have a f2f discussion to finalize the decision. This all seems quite detailed, but it's the best way to get something we can live with. If it turns out that args are going to have to be made defaultless, then that will be more complicated and will likely spawn other problems down the line.

• [ ] units since already in rsk files DK=IGNORE; CR=?; DECISION=?
• [ ] flags since already in rsk files DK=IGNORE; CR=?; DECISION=?
• [ ] missingValue since already in rsk files DK=IGNORE; CR=?; DECISION=?
• [ ] type DK=IGNORE?(not sure, but maybe rsk already has this); CR=?; DECISION=?
• [ ] serialNumber DK=IGNORE(I don't know that this makes sense to even keep. I don't think a non-RBR ctd really has a serial number, does it? I think non-rbr are just amalgams of several components, each with a serial number. But I may be wrong and ask Clark to cast the deciding vote.); CR=?; DECISION=?
• [x] ship DK=USE; CR=?; DECISION=?
• [x] cruise DK=USE; CR=?; DECISION=?`
• [x] station DK=USE; CR=?; DECISION=?
• [ ] startTime DK=IGNORE(since rsk files contain time and other ctd code should know what to do); CR=?; DECISION=?
• [x] latitude DK=USE; CR=?; DECISION=?
• [x] longitude DK=USE; CR=?; DECISION=?
• [x] deploymentType DK=?; CR=?; DECISION=? ... Clark, is this already in the rsk?
• [x] pressureAtmospheric already handled
• [ ] sampleInterval DK=IGNORE; CR=?; DECISION=?
• [ ] profile DK=IGNORE; CR=?; DECISION=?

[dankelley]

Having 10min free, and guessing there was only a small chance that @richardsc would disagree on lon and lat, so I went ahead and did them in the "develop" branch. This changes as.ctd() and rsk2ctd(). The commit is 03da24aaaa7297804defc47600d969355370c363 (and clicking that link in the web interface will show you the changed files).

[dankelley]

I did some more conversions (and clicked the checklist accordingly) in "develop" commit 7e94fef07f14c648f98626fe0dffcd4035f1fb65. Below is a test code, which I am not putting in the test suite because only an unnamed reported, @richardsc and I have the test file (which I've given a new name).

Please note my use of test_ functions, which provide a handy way to show functionality.

library(oce)
library(testthat)

rbrctd <- read.rsk("test.rsk" )

latitude <- 42.244
longitude <- -8.76

rbr <- as.ctd(rbrctd)
str(rbr[["metadata"]])
expect_true(is.na(rbr[["latitude"]]))
expect_true(is.na(rbr[["longitude"]]))
expect_equal(rbr[["serialNumber"]], 60204)
expect_equal(rbr[["model"]], "RBRconcerto")
expect_null(rbr[["deploymentType"]])

## specify some metadata.
## Note to originator (via email): we are not allowing the
## user to specify 'type', 'model', or 'serialNumber' because these
## things are stored within the .rsk file and any user who wishes
## to change them ought to be using oceSetMetadata(), to get a
## record of when and why the change was made.
rbr <- as.ctd(rbrctd,
              latitude=latitude,
              longitude=longitude,
              ship="SHIP",
              cruise="CRUISE",
              station="STATION",
              deploymentType="towyo")
str(rbr[["metadata"]])
expect_equal(rbr[["latitude"]], latitude)
expect_equal(rbr[["longitude"]], longitude)
expect_equal(rbr[["cruise"]], "CRUISE")
expect_equal(rbr[["ship"]], "SHIP")
expect_equal(rbr[["station"]], "STATION")

[dankelley]

PS. I now think we are handling all the metadata that we ought to be handling (see comments in the test file). If @richardsc disagrees, please note so here. I'm going to be busy for a couple of days and won't likely do any coding on this but we could get terribly tangled if we both worked on this complex code, so if @richardsc wants to make changes, I ask that this be done with git pull requests, to avoid merge problems. Thanks!

[dankelley]

I've done some more, and it's all documented. I think this could be closed, but that's up to @richardsc. There are tests for how this works in the build-test suite.

[dankelley]

Dear reporter -- if you think the issue (as defined by its title) has been addressed, please close it. This comment is added to an issue when it seems that an issue has been addressed and that discussion has ceased. Please bear in mind that we work on issues as defined by their titles. Narrow-scoped titles help us to focus our development efforts, and benefit other oce users.

If you think the issue has not been addressed, please look through all the comments and write a new one after this, stating what still needs to be done.

If you see that the issue has meandered from the topic defined by its title, please consider closing this and opening a new issue.

Thanks!

[richardsc]

Sorry, busy week! Looks good.