dankelley / oce

R package for oceanographic processing
http://dankelley.github.io/oce/
GNU General Public License v3.0
145 stars 42 forks source link

plotProfile() plim and ylim not working when xlim is specified #1371

Closed clayton33 closed 6 years ago

clayton33 commented 6 years ago

Short summary of problem

When trying to plot a variable that is not in the list of 'xtype's for plotProfile(), limits for y-axis do not change when plim is specified, when plim and xlim are specified together, and an error is produced when ylim and xlim are specified together. Test file and example code sent in an e-mail since the ctd profile used in the plotProfile() example does not include additional variables.

UPDATE : Here's the code

rm(list=ls())
library(oce)
file <- 'CTD_HUD2014004_186_01_DN.ODF'
d <- read.ctd(file = file)

plim <- c(500, 0) # unrealistic limits for these data
Tlim <- c(-1,12)
o2lim <- c(0,12)

par(mfrow = c(2,6))
#test plim, ylim for temperature
plotProfile(d, xtype = 'temperature')
mtext(side = 1, text = 'no limits', line = 1)

plotProfile(d, xtype = 'temperature', plim = plim)
mtext(side = 1, text = 'plim', line = 1)
plotProfile(d, xtype = 'temperature', ylim = plim)
mtext(side = 1, text = 'ylim', line = 1)

plotProfile(d, xtype = 'temperature', Tlim = Tlim)
mtext(side = 1, text = 'xlim/Tlim', line = 1)

plotProfile(d, xtype = 'temperature', plim = plim, Tlim = Tlim)
mtext(side = 1, text = 'plim + \n Tlim/xlim', line = 2)
plotProfile(d, xtype = 'temperature', ylim = plim, Tlim = Tlim)
mtext(side = 1, text = 'ylim + \n Tlim/xlim', line = 2)

#test plim, ylim for oxygen
plotProfile(d, xtype = 'oxygen')

plotProfile(d, xtype = 'oxygen', plim = plim)
plotProfile(d, xtype = 'oxygen', ylim = plim)

plotProfile(d, xtype = 'oxygen', xlim = o2lim)

plotProfile(d, xtype = 'oxygen', plim = plim, xlim = o2lim)
plotProfile(d, xtype = 'oxygen', ylim = plim, xlim = o2lim)

Output from sessionInfo()

R version 3.4.3 (2017-11-30) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Linux Mint 18

Matrix products: default BLAS: /usr/lib/libblas/libblas.so.3.6.0 LAPACK: /usr/lib/lapack/liblapack.so.3.6.0

locale: [1] LC_CTYPE=en_CA.UTF-8 LC_NUMERIC=C LC_TIME=en_CA.UTF-8
[4] LC_COLLATE=en_CA.UTF-8 LC_MONETARY=en_CA.UTF-8 LC_MESSAGES=en_CA.UTF-8
[7] LC_PAPER=en_CA.UTF-8 LC_NAME=C LC_ADDRESS=C
[10] LC_TELEPHONE=C LC_MEASUREMENT=en_CA.UTF-8 LC_IDENTIFICATION=C

attached base packages: [1] grid stats graphics grDevices utils datasets methods base

other attached packages: [1] smatr_3.4-3 plyr_1.8.4 ocedata_0.1.4 fields_9.0 maps_3.2.0
[6] spam_2.1-2 dotCall64_0.9-5 marmap_0.9.6 oce_0.9-23 gsw_1.0-5
[11] testthat_2.0.0

loaded via a namespace (and not attached): [1] Rcpp_0.12.14 raster_2.6-7 magrittr_1.5 bit_1.1-12
[5] lattice_0.20-35 R6_2.2.2 rlang_0.1.6 adehabitatMA_0.3.12 [9] stringr_1.2.0 blob_1.1.0 tools_3.4.3 DBI_0.7
[13] bit64_0.9-7 digest_0.6.13 tibble_1.4.1 reshape2_1.4.3
[17] ncdf4_1.16 memoise_1.1.0 shape_1.4.3 RSQLite_2.0
[21] sp_1.2-5 stringi_1.1.6 compiler_3.4.3 pillar_1.0.1

richardsc commented 6 years ago

I made an example using data(ctd), and it seems to work ok:

library(oce)
data(ctd)
ctd <- oceSetData(ctd, 'stupid', rnorm(ctd[['pressure']]))
plotProfile(ctd, xtype='stupid', xlim=c(0, 3), plim=c(30, 0))

image

clayton33 commented 6 years ago

After a f2f and additional investigation with @richardsc , we believe it might be due to all of the special cases in the code for plotProfile. @richardsc case using the variable 'stupid' worked because it doesn't fall under one of the special cases in the plotProfile function, whereas 'oxygen' is a special case.

library(oce)
data(ctd)
ctd <- oceSetData(ctd, 'oxygen', rnorm(ctd[['pressure']]), unit ='none')
ctd <- oceSetData(ctd, 'stupid', rnorm(ctd[['pressure']]), unit ='none')

plim <- c(100,0)
xlim <- c(0,3)

par(mfrow = c(2,6))
plotProfile(ctd, xtype = 'stupid')
mtext(side = 1, text = 'no limits', line = 1)

plotProfile(ctd, xtype = 'stupid', plim = plim)
mtext(side = 1, text = 'plim', line = 1)
plotProfile(ctd, xtype = 'stupid', ylim = plim)
mtext(side = 1, text = 'ylim', line = 1)

plotProfile(ctd, xtype = 'stupid', xlim = xlim)
mtext(side = 1, text = 'xlim', line = 1)

plotProfile(ctd, xtype = 'stupid', plim = plim, xlim = xlim)
mtext(side = 1, text = 'plim + xlim', line = 1)
plotProfile(ctd, xtype = 'stupid', ylim = plim, xlim = xlim)
mtext(side = 1, text = 'ylim + xlim', line = 1)

plotProfile(ctd, xtype = 'oxygen')

plotProfile(ctd, xtype = 'oxygen', plim = plim)
plotProfile(ctd, xtype = 'oxygen', ylim = plim)

plotProfile(ctd, xtype = 'oxygen', xlim = xlim)

plotProfile(ctd, xtype = 'oxygen', plim = plim, xlim = xlim)
plotProfile(ctd, xtype = 'oxygen', ylim = plim, xlim = xlim)
dankelley commented 6 years ago

I'll fix it after 2PM sometime. I actually made a note this morning to go into the code for "oxygen" because it made the following pic in OAR, and I want it to say "Oxygen".

screen shot 2018-01-24 at 10 49 21 am

clayton33 commented 6 years ago

Ok, no rush.

On the subject of oxygen, I noticed the same for oxygen [ml/l]. Also, for resizableLabel('oxygen mL/L'), it uses captial L's, whereas if you plot a profile, it uses lowercase L's in the unit, not sure of the convention, or if people are picky, or how often its used elsewhere in oce.

dankelley commented 6 years ago

I set up an entry in the test suite (https://github.com/dankelley/oce-issues/blob/master/13xx/1371/1371a.R) and see that it also dies, owing to a contradiction between user-supplied ylim and code-inferred ylim.

> plotProfile(ctd, xtype = 'oxygen', xlim = xlim)
> 
> plotProfile(ctd, xtype = 'oxygen', plim = plim, xlim = xlim)
> plotProfile(ctd, xtype = 'oxygen', ylim = plim, xlim = xlim)
Error in plot.default(xvar[look], y[look], xlim = xlim, ylim = ylim, lty = lty, 
 : 
  formal argument "xlim" matched by multiple actual arguments
dankelley commented 6 years ago

This problem of 'matched by multiple actual arguments' would be solved if plotProfile took xlim and ylim as formal arguments. I think it should, actually, but I need to think about that. Meantime, I'll look for what's going on with oxygen.

dankelley commented 6 years ago

I'm always keen to have bug demonstrations that tests themselves. That's easy with numbers, but hard with graphs! Can I get the reporter(s) to check https://github.com/dankelley/oce-issues/blob/master/13xx/1371/1371a.R to see whether the purple 'expect' text items are correct? From what I can see, all tests work except the last one, which I marked {FAIL}. Oh, and there's the very last one (commented out at the moment) which causes a stop but I know what causes that.

This looks like it will be a great test suite. PS. the 'look at purple text and see if it is true' method is not the only way to go here. We could also use numerical tests on par("usr"), and I may actually do that, if I don't hear back on this question fairly soon. (I'll wait 1/2 hour and after that I'll assume I'm right and start trying to fix that. I am pretty sure I'll be doing this by making xlim and ylim formal arguments to the function in question, which removes some of the wilder tangles in the code, owing to contradiction between ... and named arguments)

clayton33 commented 6 years ago

Test 8 should be marked as {FAIL} Test 10 should read mtext(side=1, text='10) EXPECT: 0<p<46; 0<x<3 {OK}', line=0, col='magenta')

dankelley commented 6 years ago

Thanks. I'm going to burn these tests into the code using expect_equal because then I can add this to the build-test suite for oce, which will mean that the problem can never recur (well, once I fix it).

dankelley commented 6 years ago

I think this is fixed in "develop" commit 13df621e3d87b68d144c2da6fe6288718be895f0. Note that I've added some extra tests to the oce-issues test code (https://github.com/dankelley/oce-issues/blob/master/13xx/1371/1371a.R) and -- more importantly -- burned these tests into the oce build suite. Now, in RStudio, if you build up oce and then click "Build -> More > Test Package" you'll get these new tests. The tests are in tests/testthat/test_local_plotProfile.R, and the fact that "local" is in the filename means that these tests are not performed on CRAN, but only locally. (I do slow tests only locally ... if the algorithm works on my machine, it should work on all machines. I also have a test suite that runs through over a 1000 CTD files, which of course we don't want being incorporated into oce!)

Please have a look at tests/testthat/test_local_plotProfile.R to see if it makes sense, in addition to seeing if your own code works, before closing this.

PS. in tests/testthat/test_local_plotProfile.R I have some nice error messages only on the cases near the bottom, which I wrote a few minutes ago. I guess it would make sense to look at the others and try to find a sentence describing each, so if others in this discussion want to do that, feel free to make a pull request.