make get.vertex.attribute and list.vertex.attributes generics

[chad-klumb]

PR to convert a couple of accessors to generics.

Ran revdeps on statnet2 for current network master and also the version of network in this PR. Only change in results was a new warning and a new note for networkDynamic:

* checking S3 generic/method consistency ... WARNING
get.vertex.attribute:
  function(x, attrname, ...)
get.vertex.attribute.active:
  function(x, prefix, onset, terminus, length, at, rule, na.omit,
           null.na, active.default, dynamic.only, require.active,
           return.tea, unlist)

list.vertex.attributes:
  function(x, ...)
list.vertex.attributes.active:
  function(x, onset, terminus, length, at, na.omit, rule, v,
           require.active, active.default, dynamic.only)

See section ‘Generic functions and methods’ in the ‘Writing R
Extensions’ manual.

Found the following apparent S3 methods exported but not registered:
  get.vertex.attribute.active list.vertex.attributes.active
See section ‘Registering S3 methods’ in the ‘Writing R Extensions’
manual.

...

* checking Rd \usage sections ... NOTE
S3 methods shown with full name in documentation object 'attribute.activity.functions':
  ‘get.vertex.attribute.active’ ‘list.vertex.attributes.active’

The \usage entries for S3 methods should use the \method markup and not
their full name.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.

With get.vertex.attribute and list.vertex.attributes being made generic, the networkDynamic functions get.vertex.attribute.active and list.vertex.attributes.active "appear" to be S3methods, hence the warning/note. (The tests passed, anyway.) It looks like similar warnings were addressed in the past, e.g. in statnet/networkDynamic@651823a, by adding S3method lines to networkDynamic's NAMESPACE, adding an ellipsis to the function arguments, and tweaking the docs to use \method.

The one hitch in this case is that networkDynamic's get.vertex.attribute.active doesn't take the attrname argument. To quash the warning in this case, we may need to remove attrname from the generic. I don't expect that would cause behavioral problems (I can re-run tests with that change to make sure), but would modify user-facing content somewhat. What do people think about how to handle this issue?

Revdep summary for current network on statnet2:

Check status summary:
                  ERROR NOTE OK
  Source packages     0    0  1
  Reverse depends    18   17 47

Summary for proposed network on statnet2 (the OK -> WARN was networkDynamic):

Check status summary:
                  ERROR WARN NOTE OK
  Source packages     0    0    0  1
  Reverse depends    18    1   17 46

Since many revdeps errored due to dependencies (often RSiena) not being installable on statnet2, I also tested on a windows machine (which supported RSiena, but had issues with vignettes, so the windows tests do not include vignettes).

Summary for current network on windows (with no vignettes, which is what caused the warning for network):

Check status summary:
                  ERROR WARN NOTE OK
  Source packages     0    1    0  0
  Reverse depends     7    0   16 59  

Summary for proposed network on windows:

Check status summary:
                  ERROR WARN NOTE OK
  Source packages     0    1    0  0
  Reverse depends     7    1   16 58

(Again, the OK -> WARN was networkDynamic.)

All revdep errors above were related to dependencies of the revdep or the revdep itself not being installable (they weren't test failures).

I was hoping to merge this in about 24 hours but we need to decide what to do about the generic argument first. So, interested parties, please let me know what you'd prefer to do here.

[sgoodreau]

I wish I had all the knowledge needed to be able to respond intelligently and usefully to your questions here, @chad-klumb. Alas I will need to leave it to @krivit and @martinamorris to do so.

[chad-klumb]

windows revdeps with attrname removed finished: same results as before (which is good)

Check status summary:
                  ERROR WARN NOTE OK
  Source packages     0    1    0  0
  Reverse depends     7    1   16 58

statnet2 ran out of memory so I'm deleting some stuff and restarting the tests there

also separately made changes to networkDynamic needed to work with new version of network and it passed R CMD check; I can make a PR for those as well if people want (just lmk)

assuming statnet2 tests come back same as before I will add the removal of attrname to this PR (unless there are stated objections); it's a little weird for the generic get.vertex.attribute to not have an argument stating what attribute to get, but if people look at the network class method (which is on the same man page) they'll see attrname there

[skyebend]

this seems like a great!, ... I thought there were some gotchas when we looked at this before, but don't see any, can you do PR for the network and networkDynamic changes?

[chad-klumb]

statnet2 results were also the same with attrname removed:

Check status summary:
                  ERROR WARN NOTE OK
  Source packages     0    0    0  1
  Reverse depends    18    1   17 46

so I've added commits to remove attrname from this PR

also made a PR for changes to networkDynamic

some Travis builds for networkDynamic initially failed (for what appeared to be unrelated reasons); I triggered rebuilds and they all passed this time around

I think we are now in a position to merge #14 into network@master and statnet/networkDynamic#2 into networkDynamic@master (at which point we'll want to update networkDynamic's travis.yml to point to network@master rather than my fork)

If there are any questions or lingering concerns about these PRs please let me know.

[skyebend]

If the attrname is "optional", does it fail cleanly and informatively if it is omitted?

[chad-klumb]

Same error message when omitting attrname, either with CRAN network or the version in this PR:

require(ergm)
x <- network(10,dir=F)
x %v% "a" <- runif(10)
get.vertex.attribute(x)
Error in lapply(x$val, "[[", attrname) : 
  argument "attrname" is missing, with no default

It could possibly be made more informative but I think it's already pretty reasonable.

[skyebend]

message seems very clear! :-) If this is going to master, can you increment the (minor) version and changelog?

[chad-klumb]

message seems very clear! :-) If this is going to master, can you increment the (minor) version and changelog?

done

[chad-klumb]

merging with blessings from Carter (via email)