search

jakobbossek / mcMST

Algorithms to solve the multi-criteria minimum spanning tree problem (mcMST) in R
Other
4 stars 2 forks source link

Coding standards/misc #11

Closed gvegayon closed 6 years ago

gvegayon commented 6 years ago

I know that there is no golden rule/book of coding conventions in R. But I just thought about mentioning these that you can consider to add in your project (which overall looks very good :+1: ), plus a couple of other misc comments:

  1. Following Hadley Wickham's R-pkgs

    It’s common for packages to be listed in Imports in DESCRIPTION, but not in NAMESPACE. In fact, this is what I recommend: list the package in DESCRIPTION so that it’s installed, then always refer to it explicitly with pkg::fun(). Unless there is a strong reason not to, it’s better to be explicit. It’s a little more work to write, but a lot easier to read when you come back to the code in the future.

    http://r-pkgs.had.co.nz/namespace.html

    I would use the :: notation instead of, for example, requirePackages in plotGraph.R I would directly call ggplot2::ggplot and friends.

  2. Since you already have the print method for mcGP objects, you can introduce the plot.mcGP method as well instead of the plotGraph function. Sort of the same thing applies with the objects of class ecr_multi_objective_result ecr_result

  3. I miss some examples in: edgeListToCharVec, permutationToCharVec, permutationToEdgelist, prueferToCharVec

  4. Not an expert in the problem of multiple objective minimum spanning tree problems, but after googling a on CRAN I found these packages that are related and perhaps can be nice to include around references:

jakobbossek commented 6 years ago

Thanks @gvegayon for all the valuable comments! 👏

ad 1) Thanks for pointing this out. Did not know about that recommendation yet. I would change this in all my projects later. Is this a blocker in your eyes? ad 2) I get the point. However, plot.mcGP would expect to use base R graphics. I rather prefer ggplot2 graphics. Yes, in this case I could provide autoplot.mcGP. However, this function is expected to return an object of type gg/ggplot, but plotGraph is composed of two graphics side by side in some cases, which would require to return a list of ggplot objects or a ggtable object. This is why I strongly prefer not to change this. ad 3) added examples to all transformation functions. ad 4) Thanks for the package list. Added a section on related work to the README.

gvegayon commented 6 years ago

Is this a blocker in your eyes?

Not at all, as I said, it is about style. I do think that HW's suggestion should be a standard overall, but if your package works and CRAN is OK with it, I should be OK as well.

plot.mcGP would expect to use base R graphics

Why is that? I'm not a ggplot2 user, but I think there is no requirement for plot.[someclass]... it is actually very flexible. In the case of ggplot2 it suffices with returning the ggplot object and R will do the rest (no need of explicitly calling print). Here is an example of an R package that I've been working on that uses ggplot2 (actually ggtree) in a plot S3 method. Furthermore. I've just submitted a pull request with this so you can take a look at it: https://github.com/jakobbossek/mcMST/pull/12

added examples to all transformation functions.

Cool!

Thanks for the package list. Added a section on related work to the README.

You are welcomed

Also, I just realized that there is no manual for mcMST or mcMST-package. I suggest you create one... That would be #5

Also (2), I added a ChangeLog... I think these are useful

HIH

jakobbossek commented 6 years ago

plot.mcGP would expect to use base R graphics

Why is that? I'm not a ggplot2 user, but I think there is no requirement for plot.[someclass]... it is actually very flexible. In the case of ggplot2 it suffices with returning the ggplot object and R will do the rest (no need of explicitly calling print). Here is an example of an R package that I've been working on that uses ggplot2 (actually ggtree) in a plot S3 method. Furthermore. I've just submitted a pull request with this so you can take a look at it: #12

Thanks a lot @gvegayon! Merged.

Also, I just realized that there is no manual for mcMST or mcMST-package. I suggest you create one... That would be #5

Just added docs for the package via 9dfbeaf 👍

Also (2), I added a ChangeLog... I think these are useful

Thanks!

I guess everything is fine now 😃

gvegayon commented 6 years ago

Indeed