Watch out for S3 classes!

[sje30]

When testing the new code (inst/sje1.R) against my old code I noticed the following problem:

ns1 <- sjemea::compute.ns(s, ns.T=0.003, ns.N=10,sur=100)
ns2 <- .compute_ns(s, ns_t=0.003, ns_n=10,sur=100)
plot(ns1)
plot_network_spikes(ns2) ## Note problem: this should  be just plot(ns2)

The two objects ns1 and ns2 are both of S3 class "ns" (short for network spikes). S3 classes work in a very dumb way, so e.g. if you have an object x of class "ns" then if you run plot(x) it will look for the function plot.ns(x,...)

so my call to plot(ns1) works fine because in sjemea package the plotting function is called plot.ns()
but in the MI branch I see you have change the plotting function to plot_network_spikes()

so by changing all . to _ we've lost S3 classes... I'll check what other classes might be a problem...

[sje30]

it looks like the only call to generating classes in meaRtools is the network spikes and the axion code:

  class(layout) <- "mealayout"

and I can't see the corresponding plotting functions, so it may just be the network spikes.

Would you be okay then if we renamed plot_network_spikes back to plot.ns so that the S3 class works still (or do you prefer to work with another class framework, like S4 or S6, for meaRtools)?

[QuanliWang]

Sure. The only concern is the plot.ns will not pass lintr, as dot is not part of snake case. On Jul 27, 2017, at 5:14 AM, Stephen Eglen <notifications@github.com<mailto:notifications@github.com>> wrote: it looks like the only call to generating classes in meaRtools is the network spikes and the axion code: class(layout) <- "mealayout" and I can't see the corresponding plotting functions, so it may just be the network spikes. Would you be okay then if we renamed plot_network_spikes back to plot.ns so that the S3 class works still (or do you prefer to work with another class framework, like S4 or S6, for meaRtools)? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub<#30 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHcczEXV4UNEhSyfedP1uRekgPMR3GtKks5sSFT6gaJpZM4Ok_mr>.

[sje30]

Thanks. I see this is on the list for lintr: r-lib/lintr#223

so in the meantime, we can put a #nolint comment to quieten lint.