hoffmangroup / segtools

Package for analysis of genomic segmentations.
http://pmgenomics.ca/hoffmanlab/proj/segtools
GNU General Public License v2.0
0 stars 0 forks source link

R error: "nlabels not found" in version 1.1.9 #24

Closed EricR86 closed 9 years ago

EricR86 commented 12 years ago

Original report (archived issue) by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Original issue 24 created by ericr86 on 2012-01-10T16:33:36.000Z:

Running segtools-gmtk-parameters, version 1.1.9, results in the error : object 'nlabels' not found.

The error is in the definition of the function save.track.stats in track_statistics.R, line 631.

Restoring the definition from segtools version 1.1.7, corrects the issue, as nlabels and ntracks are defined within the function.

EricR86 commented 10 years ago

Original comment by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Comment #1 originally posted by ericr86 on 2014-03-30T21:25:15.000Z:

Is there a diff for this somewhere?

EricR86 commented 10 years ago

Original comment by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Comment #2 originally posted by ericr86 on 2014-03-31T18:33:49.000Z:

I just uploaded the Segtools repo to here:

!/hoffmanlab/segtools

Sadly, version tags did not seem to be included in the conversion for SVN (they are for Segway). You can find the Segtools 1.1.7 source linked from

https://pypi.python.org/pypi/segtools/1.1.7

EricR86 commented 9 years ago

Original comment by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Tags were not preserved evidently on the transition from SVN to mercurial.

The last revision that I can find where track_statistics.R matches is with revision 176:53927fbc27bda83c0ae23cbba12b6f9da692e9a5. There are 17 changes to track_statistics.R between then and now (the last being on 272:cf7c6f62cf1f6ce6d7d7a3e60e702c24575ec345).

The error comes from revision 223:66a8dde3a471b4d36bddeb30b00ec1383c7d0933

$ hg log -r 223
changeset:   223:66a8dde3a471b4d36bddeb30b00ec1383c7d0933
user:        Orion Buske <buske@cs.toronto.edu>
date:        Thu Oct 13 01:30:02 2011 +0000
files:       segtools/R/track_statistics.R
description:
Made width specifiable in in track stats plotting

$ hg diff -c 223 segtools/R/track_statistics.R
diff -r 3258d3bcba65110d06ba6d2b9e040d13e9492506 -r 66a8dde3a471b4d36bddeb30b00ec1383c7d0933 segtools/R/track_statistics.R
--- a/segtools/R/track_statistics.R     Wed Oct 12 17:17:27 2011 +0000
+++ b/segtools/R/track_statistics.R     Thu Oct 13 01:30:02 2011 +0000
@@ -567,7 +567,7 @@
     col.ord <- match(label_order, colnames(means))
     stopifnot(col.ord > 0, length(col.ord) == ncol(means))
   }
-  par(oma = c(1, 1, 1, 1))  # Add a margin
+  #par(oma = c(1, 1, 1, 1))  # Add a margin

   sds.ordered = NULL
   if (! is.null(sds)) {
@@ -627,6 +627,10 @@
                              symmetric = FALSE,
                              clobber = FALSE,
                              square.size = 15,  # px
+                             height = 200 + square.size * ntracks,
+                             width = 450 + square.size * nlabels,
+                             height.pdf = height / 72,
+                             width.pdf = width / 72,
                              ...) {
   mnemonics <- read.mnemonics(mnemonic_file)
   translations <- read.mnemonics(translation_file)
@@ -637,13 +641,13 @@

   ntracks <- nlevels(res$stats$trackname)
   nlabels <- nlevels(res$stats$label)
-  height <- 200 + square.size * ntracks
-  width <- 450 + square.size * nlabels
-
+
   save.images(dirpath, namebase,
               levelplot.track.stats(res, symmetric = symmetric, ...),
               height = height,
               width = width,
+              height.pdf = height.pdf,
+              width.pdf = width.pdf,
               clobber = clobber)
 }

Essentially the defined width/height parameters were copy pasted into the parameters probably not realizing that using nlabels in the default parameters was a bad idea. There's a good chance it was never caught if the function was always called with non-default width and height parameters.

This was never fixed in later histories (and then accidentally reverted).

It does raise the question as to what the default width/height parameters should be.

EricR86 commented 9 years ago

Original comment by Michael Hoffman (Bitbucket: hoffman, GitHub: michaelmhoffman).


Can you fix it with something like height = NULL, width = NULL, height.pdf = NULL, width.pdf = NULL in the argument list?

Then after ntracks and nlabels are defined you can can do something like if (is.null(height)) height <- 200 + square.size * ntracks? (Better to have that 200 as another argument if so.)

EricR86 commented 9 years ago

Original comment by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Can you clarify why you would like the 200 as a default for a function argument? If you're referring to the default that's already there I don't see why it couldn't be used in the default when height = NULL.

EricR86 commented 9 years ago

Original comment by Michael Hoffman (Bitbucket: hoffman, GitHub: michaelmhoffman).


As a general rule, we shouldn't have magic numbers like that in the body of a function. A constant at the top of the file is fine.

EricR86 commented 9 years ago

Original comment by Eric Roberts (Bitbucket: ericr86, GitHub: ericr86).


Fixed by commit d46049a6e1f18a462cdac8eb5505046f50e41324