On 09/30/2016 09:53 PM, Vincent Carey wrote:
i would have comments on StarTrek. if you would like me to be reviewer
of that i can do it.
a query on policy
how do you propose to handle needlessly repeated code?
It can be very helpful to indicate sub-optimal approaches to coding to
the authors, and certainly in this case.
There is the risk of an analog of 'premature optimization', where the
code is implemented inefficiently / poorly but it does not matter for
the overall functioning of the package; it can be hard to know from a
more-or-less superficial look at the package when one crosses the line
from helpful to pedant. I find it useful to think of a (reviewer) time
budget, and maximizing the value I add to the package for the amount of
time I invest.
the code given below is repeated many many times with slight variations
for selected KEGG_path. i would ask the author
to implement DRY (don't repeat yourself), find a functional approach to
the repeated element, and
simplify. i can imagine that damping enthusiasm of the contributor but
I think it is worth holding back
until such a thing is accomplished. too meddlesome?
if (KEGG_path=="Carb_met") {
pathways.list <- keggList("pathway", "hsa")## returns the list of human
pathways
mer<-select_path_carb(Carbohydrate)
common<-intersect(pathways.list,mer)
lo<-list()
for (i in 1:length(pathways.list)){
if (length(intersect(pathways.list[[i]],common)!=0)){
lo[[i]]<-pathways.list[[i]]
names(lo)[[i]]<-names(pathways.list)[[i]]
}
}
pathways.list<-lo[lapply(lo,length)!=0]
pathway.codes <- sub("path:", "", names(pathways.list))
a<-do.call("rbind", pathways.list)
}
Another issue the author seems to raise is the need for "open cytoscape
session" for meaningful tests.
I think this will never happen on the build system. So the author has
to emulate the cytoscape behavior
perhaps with some static objects that cytoscape might return on interaction.
Yes, in earlier times the build system would be adjusted in various ways
to accommodate package needs, but this is not realistic.
Especially for tests, one wants to emphasize functionality of one's own
code and hence replace reliance on a service with proxy objects.
Martin
On Fri, Sep 30, 2016 at 5:22 PM, Martin Morgan
<[email protected] <mailto:[email protected]>>
wrote:
On 09/30/2016 12:05 PM, Michael Lawrence wrote:
I have been chipping in here and there, but I'm not sure if I'm
causing more confusion than help. I just select packages that
interest
me and contribute bite size pieces, but I would be happy to assume a
more formal role. It takes very little of my time.
One suggestion is that reviews should be incremental, i.e., provide
obvious feedback first, let the package authors respond, then dig
deeper for more detailed comments.
Michael, I've found your comments / reviews really helpful. They are
generally on-target and constructive, and help spur the authors to
think about what they are doing. They do in some ways 'confuse' the
issue (how is the package author to know that the 'assignee' is the
official reviewer? are the people commenting on the package
providing a consistent message?), but definitely a plus. I encourage
others to participate in the public review process in this way, too.
It would be great to hear from people who would like to take on the
role of assigned reviewer. I'd suggest you identify package(s) that
would be interesting to you, and send email to me. I'll arrange for
you to become the assigned reviewer, and provide basic instructions
and guidelines. In the future, you will be able to take on a package
more directly.
From the formal review process, incremental additional comments are
not a good way to go, because the package author can feel like they
are on a never-ending treadmill of requirements. Instead I think the
formal review should more or less set out a contract -- do these
things and your package will meet reviewer requirements for
inclusion in Bioconductor. This makes it harder / more time
consuming to review the package, because there is only the one
chance to identify the most important points. Also, one needs to
follow through and confirm that yes, the appropriate changes were made.
It will be interesting to adjust the public review process,
especially after the next release. For instance, one approach I like
is to provide a review with editable check boxes, with the package
author editing the review with information about commits, etc, that
satisfy the concerns being expressed. I don't really like the canned
check-list in the joss link Tim mentions; they point to important
areas but seem too generic to lead to constructive changes to
individual packages.
Martin
Michael
On Fri, Sep 30, 2016 at 8:49 AM, Kasper Daniel Hansen
<[email protected]
<mailto:[email protected]>> wrote:
One of the advantages of the Github review process is open
review. We have
some amount of packages submitted in the last moment (and I
am guilty of
one of these). Is help with the reviews needed? If so, how
do we know
where to concentrate?
Kasper
[[alternative HTML version deleted]]
_______________________________________________
[email protected] <mailto:[email protected]>
mailing list
https://stat.ethz.ch/mailman/listinfo/bioc-devel
<https://stat.ethz.ch/mailman/listinfo/bioc-devel>
_______________________________________________
[email protected] <mailto:[email protected]>
mailing list
https://stat.ethz.ch/mailman/listinfo/bioc-devel
<https://stat.ethz.ch/mailman/listinfo/bioc-devel>
This email message may contain legally privileged and/or...{{dropped:2}}
_______________________________________________
[email protected] <mailto:[email protected]> mailing list
https://stat.ethz.ch/mailman/listinfo/bioc-devel
<https://stat.ethz.ch/mailman/listinfo/bioc-devel>
--
Vincent J. Carey, Ph.D.
Professor of Medicine
Channing Division of Network Medicine
Brigham and Women's Hospital
Harvard Medical School
181 Longwood Avenue
Boston, MA 02115 USA
This email message may contain legally privileged and/or...{{dropped:2}}
_______________________________________________
[email protected] mailing list
https://stat.ethz.ch/mailman/listinfo/bioc-devel
