Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plotProfile() should use units (if known) #981

Closed
dankelley opened this issue Jun 5, 2016 · 10 comments
Closed

plotProfile() should use units (if known) #981

dankelley opened this issue Jun 5, 2016 · 10 comments
Assignees
Labels

Comments

@dankelley
Copy link
Owner

Of course this should be done throughout oce but I don't want an issue that will stay "in play" for a month. I'd prefer to tackle it a bit at a time. Also, plotProfile() is probably the most important case to consider, because this is where the majority of the unit variation lies. Below is a screenshot demonstrating that e.g. oxygen really could use a unit!
screen shot 2016-06-05 at 6 21 41 pm

@dankelley dankelley added the ctd label Jun 5, 2016
@dankelley dankelley self-assigned this Jun 5, 2016
This was referenced Jun 8, 2016
@dankelley
Copy link
Owner Author

dankelley commented Jun 8, 2016

Some keyhole coding tests have revealed a way to do this (my test case yielded #982, since closed) and I may have time to make a provisional fix tomorrow afternoon, or Friday afternoon, or on Saturday. I really think this is quite important, in the sense that we can now read a lot of SBE things that can have whacky units, and so we can now make plots that will have incorrect units on the axes, unless I fix this. Therefore, my idea is to try to get this fixed up before early next week, then to have a week of leaving oce alone for day-to-day tests, and then a release on the June 18. Actually, we could even be a bit more cautious and release on St Jean Baptiste Day, for a little culture.

@dankelley
Copy link
Owner Author

Just because I'm logging out for the night, I want to note that we also have an issue test, as follows, and it does something good in kelley although I will recode things quite a bit to make it extend beyond nitrite. (I coded for nitrite as a keyhole test.)

library(oce)
data(section)
ctd <- section[["station", 1]]

if (!interactive()) png("981.png")

par(mfrow=c(2,1))
plotProfile(ctd, xtype="nitrite")
mtext("Expect unit [umol/kg]", side=3, line=2, adj=1, col='magenta', font=2)

ctd[["metadata"]]$units$nitrite <- list(unit=expression(nmol/g), scale="")
plotProfile(ctd, xtype="nitrite")
mtext("Expect unit [nmol/g]", side=3, line=2, adj=1, col='magenta', font=2)

if (!interactive()) dev.off()

@dankelley
Copy link
Owner Author

I think part of fixing this will also involve making resizableLabel() and permit unrecognized items. I don't see any downside in that (previously, resizableLabel() would just fail, so letting it continue won't break any existing working code) so I'll do that change without making a new issue.

@dankelley
Copy link
Owner Author

dankelley commented Jun 11, 2016

Time for a checklist of things to change in R/misc.R:resizableLabel() ... I'll tick things off as I do them in my own code, without pushing to GH until I get more done. Note that axes only show units, not scales, so e.g. the label for a salinity axis is is fixed (to degC).

  • if (item == "T") {
  • } else if (item == "conductivity mS/cm") { (irrelevant since the unit is fixed and known)
  • } else if (item == "conductivity S/m") {(irrelevant since the unit is fixed and known)
  • } else if (item == "C") { # unitless form (irrelevant since the unit is fixed and known)
  • } else if (item == "conservative temperature") {(irrelevant since the unit is fixed and known)
  • } else if (item == "sigmaTheta") {(irrelevant since the unit is fixed and known)
  • } else if (item == "theta") {(irrelevant since the unit is fixed and known)
  • } else if (item == "tritium") {
  • } else if (item == "nitrate") {
  • } else if (item == "nitrite") {
  • } else if (item == "oxygen") {
  • } else if (item == "oxygen saturation") {
  • } else if (item == "oxygen umol/L") { (irrelevant since the unit is fixed and known)
  • } else if (item == "oxygen umol/kg") { (irrelevant since the unit is fixed and known)
  • } else if (item == "phosphate") {
  • } else if (item == "silicate") {
  • } else if (item == "fluorescence") {
  • `} else if (item == "spice") {
  • } else if (item == "S") {(irrelevant since the unit is fixed and known)
  • } else if (item == "absolute salinity") { (irrelevant since the unit is fixed and known)
  • } else if (item == "p") {(irrelevant since the unit is fixed and known)
  • } else if (item == "z") {(irrelevant since the unit is fixed and known)
  • } else if (item == "distance") {(irrelevant since the unit is fixed and known)
  • } else if (item == "distance km") { (irrelevant since the unit is fixed and known)
  • } else if (item == "along-track distance km") { (irrelevant since the unit is fixed and known)
  • } else if (item == "heading") { (irrelevant since the unit is fixed and known)
  • } else if (item == "pitch") { (irrelevant since the unit is fixed and known)
  • } else if (item == "roll") { (irrelevant since the unit is fixed and known)
  • } else if (item == "u" || item == "v" || item == "w") { (irrelevant since the unit is fixed and known)
  • } else if (item == "eastward") {(irrelevant since the unit is fixed and known)
  • } else if (item == "northward") {(irrelevant since the unit is fixed and known)
  • } else if (item == "depth") {(irrelevant since the unit is fixed and known)
  • } else if (item == "elevation") {(irrelevant since the unit is fixed and known)
  • } else if (item == "latitude") {(irrelevant since the unit is fixed and known)
  • } else if (item == "longitude") {(irrelevant since the unit is fixed and known)
  • } else if (item == "frequency cph") {(irrelevant since the unit is fixed and known)
  • } else if (item == "spectral density m2/cph") {(irrelevant since the unit is fixed and known)

@dankelley
Copy link
Owner Author

It's not often I reference my blog here, but in this case I will, because it shows a solution I found after 2 hours of saying bad words at my computer: blog

@dankelley
Copy link
Owner Author

dankelley commented Jun 11, 2016

I've made progress in the kelley branch; commit note below, and test code in 981a.R

commit c3c3a16
Author: Dan Kelley kelley.dan@gmail.com
Date: Sat Jun 11 15:23:00 2016 -0300

Re issue 981, plotProfile() handles units for:

* temperature
* salinity
* nitrite
* oxygen
* phosphate
* silicate
* NO2+NO3
* new columns created with ctdAddColumn()

@dankelley
Copy link
Owner Author

dankelley commented Jun 11, 2016

Also, and this is confusing because I can only test a few things with 981a.R, plotProfile() handles fluorescence and several other things listed in previous comment. I think I'll merge kelley into develop after I try some more tests tomorrow morning, but 981a.R is probably enough to know this has been done correctly.

@dankelley
Copy link
Owner Author

I've merged kelley into develop, since this seems ok and also since (by mistake!) I fixed up some broken URLs in kelley.

@dankelley
Copy link
Owner Author

I am not planning on altering plotTS() because it's too complicated to figure out the right thing, given that the isopycnals are computed on the expectation of degC. (Really, the present issue relates more to things like oxygen, silicate, etc., where there really is some variation in units ... if someone wants to set up their SBE processing to output degF, then they really should be sensible enough to add a new column that holds degC, etc.)

@dankelley
Copy link
Owner Author

I'm closing this, and opening one for plot.section(), which I see doesn't even show units!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant