New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review request: Petchey, Plebani, Pennekamp #15

Closed
wants to merge 62 commits into
from

Conversation

Projects
None yet
5 participants
@opetchey

Dear @ReScience/editors,

I request a review for the reproduction of the following paper:

Best wishes
Owen Petchey


EDITOR

  • Editor acknowledgment (@tpoisot) Feb 23, 2016
  • Reviewer 1 (@yoavram) March 3, 2016
  • Reviewer 2 (@FedericoV) March 3, 2016
  • Review 1 decision [accept] April 5, 2016
  • Review 2 decision [accept] April 13, 2016
  • Editor decision [accept] April 13, 2016
@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Mar 29, 2016

Please ignore this comment, and see below.

Thanks @yoavram
Following is a commit in which I've addressed all your minor issues; thank you very much for pointing them out. Only the figure 2 & 4 issue I am not sure if I fixed.
The major issue I attempted to fix. Please check this.
(Apologies I can't be sure of some of the fixes, but your computer / R is behaving differently from mine, and I can't figure how to make mine behave the same as yours. At least one difference is how dplyr::gather() converts the key variable to factor variable for me, but leaves it as a character variable for you.)

Please ignore this comment, and see below.

Thanks @yoavram
Following is a commit in which I've addressed all your minor issues; thank you very much for pointing them out. Only the figure 2 & 4 issue I am not sure if I fixed.
The major issue I attempted to fix. Please check this.
(Apologies I can't be sure of some of the fixes, but your computer / R is behaving differently from mine, and I can't figure how to make mine behave the same as yours. At least one difference is how dplyr::gather() converts the key variable to factor variable for me, but leaves it as a character variable for you.)

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Mar 29, 2016

@opetchey / @yoavram does either of you have a .Rprofile (or equivalent) file, setting default options?

Mine has the line

options(stringsAsFactors=FALSE)

and it changes the way strings in data.frames are handled.

tpoisot commented Mar 29, 2016

@opetchey / @yoavram does either of you have a .Rprofile (or equivalent) file, setting default options?

Mine has the line

options(stringsAsFactors=FALSE)

and it changes the way strings in data.frames are handled.

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Mar 29, 2016

Thanks @yoavram
Following is a commit in which I've addressed all your minor issues; thank you very much for pointing them out. Only the figure 2 & 4 issue I am not sure if I fixed.
The major issue I attempted to fix. Please check this.
(Apologies I can't be sure of some of the fixes, but your computer / R is behaving differently from mine, and I can't figure how to make mine behave the same as yours. At least one difference is how dplyr::gather() converts the key variable to factor variable for me, but leaves it as a character variable for you.)

@tpoisot I think the problem was in the gather() function of dplyr, the behaviour of which is not controlled by stringsAsFactors. But in the latest fix, I think I specified the behaviour of gather() explicitly, and that hopefully this fixes the difference in behaviour.

Thanks @yoavram
Following is a commit in which I've addressed all your minor issues; thank you very much for pointing them out. Only the figure 2 & 4 issue I am not sure if I fixed.
The major issue I attempted to fix. Please check this.
(Apologies I can't be sure of some of the fixes, but your computer / R is behaving differently from mine, and I can't figure how to make mine behave the same as yours. At least one difference is how dplyr::gather() converts the key variable to factor variable for me, but leaves it as a character variable for you.)

@tpoisot I think the problem was in the gather() function of dplyr, the behaviour of which is not controlled by stringsAsFactors. But in the latest fix, I think I specified the behaviour of gather() explicitly, and that hopefully this fixes the difference in behaviour.

@yoavram

This comment has been minimized.

Show comment
Hide comment
@yoavram

yoavram Mar 31, 2016

I pulled and ran again, panels are still sorted alphabetically.

yoavram commented Mar 31, 2016

I pulled and ran again, panels are still sorted alphabetically.

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Mar 31, 2016

@yoavram -- reproducting the manuscript is not as important as reproducing the contents of the figures -- @rougier can you confirm?

If you managed to reproduce the contents of the figures, this will be sufficient.

tpoisot commented Mar 31, 2016

@yoavram -- reproducting the manuscript is not as important as reproducing the contents of the figures -- @rougier can you confirm?

If you managed to reproduce the contents of the figures, this will be sufficient.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Mar 31, 2016

Member

Yes, the main point of the review is first to check whether original results can be replicated (or not) using the proposed new implementation. But ensuring anyone can re-run the replication is equally important of course. Concerning possible layout issues in the new manuscript, we can try to fix them after the review if the paper is accepted.

Member

rougier commented Mar 31, 2016

Yes, the main point of the review is first to check whether original results can be replicated (or not) using the proposed new implementation. But ensuring anyone can re-run the replication is equally important of course. Concerning possible layout issues in the new manuscript, we can try to fix them after the review if the paper is accepted.

@yoavram

This comment has been minimized.

Show comment
Hide comment
@yoavram

yoavram Mar 31, 2016

Sure, I agree. I only listed it as a minor issue because I got a layout that was different from the one the authors got.

yoavram commented Mar 31, 2016

Sure, I agree. I only listed it as a minor issue because I got a layout that was different from the one the authors got.

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 2, 2016

@yoavram please can you send me an image of your workspace after all the code is run?
And also what appears in the console if you run type.convert. E.g., I get:

> type.convert
function (x, na.strings = "NA", as.is = FALSE, dec = ".", numerals = c("allow.loss", 
    "warn.loss", "no.loss")) 
.External2(C_typeconvert, x, na.strings, as.is, dec, match.arg(numerals))
<bytecode: 0x7fa355108d40>
<environment: namespace:utils>

opetchey commented Apr 2, 2016

@yoavram please can you send me an image of your workspace after all the code is run?
And also what appears in the console if you run type.convert. E.g., I get:

> type.convert
function (x, na.strings = "NA", as.is = FALSE, dec = ".", numerals = c("allow.loss", 
    "warn.loss", "no.loss")) 
.External2(C_typeconvert, x, na.strings, as.is, dec, match.arg(numerals))
<bytecode: 0x7fa355108d40>
<environment: namespace:utils>
@yoavram

This comment has been minimized.

Show comment
Hide comment
@yoavram

yoavram Apr 3, 2016

Sure! I get what you get, I think:

> type.convert
function (x, na.strings = "NA", as.is = FALSE, dec = ".", numerals = c("allow.loss", 
    "warn.loss", "no.loss")) 
.External2(C_typeconvert, x, na.strings, as.is, dec, match.arg(numerals))
<bytecode: 0x000000001be04680>
<environment: namespace:utils>

See attached zipped RData file

opetchey.zip

yoavram commented Apr 3, 2016

Sure! I get what you get, I think:

> type.convert
function (x, na.strings = "NA", as.is = FALSE, dec = ".", numerals = c("allow.loss", 
    "warn.loss", "no.loss")) 
.External2(C_typeconvert, x, na.strings, as.is, dec, match.arg(numerals))
<bytecode: 0x000000001be04680>
<environment: namespace:utils>

See attached zipped RData file

opetchey.zip

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 4, 2016

Yes, I still don't know why we're getting different results. But something to do with treatment of character variables.
In any case, I attempted a further improvement... see commit 860500c

opetchey commented Apr 4, 2016

Yes, I still don't know why we're getting different results. But something to do with treatment of character variables.
In any case, I attempted a further improvement... see commit 860500c

@yoavram

This comment has been minimized.

Show comment
Hide comment
@yoavram

yoavram Apr 4, 2016

REVIEWER 1

As far as I'm concerned and following what @tpoisot wrote above (31 Marh 2016) this is a minor issue. I believe the ms can be accepted.

yoavram commented Apr 4, 2016

REVIEWER 1

As far as I'm concerned and following what @tpoisot wrote above (31 Marh 2016) this is a minor issue. I believe the ms can be accepted.

@FedericoV

This comment has been minimized.

Show comment
Hide comment
@FedericoV

FedericoV Apr 4, 2016

I've been very busy but I will be able to handle this tomorrow. Will
update and reproduce all the steps.

On Mon, 4 Apr 2016 at 21:59 Yoav Ram notifications@github.com wrote:

REVIEWER 1

As far as I'm concerned and following what @tpoisot
https://github.com/tpoisot wrote above (31 Marh 2016) this is a minor
issue. I believe the ms can be accepted.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#15 (comment)

I've been very busy but I will be able to handle this tomorrow. Will
update and reproduce all the steps.

On Mon, 4 Apr 2016 at 21:59 Yoav Ram notifications@github.com wrote:

REVIEWER 1

As far as I'm concerned and following what @tpoisot
https://github.com/tpoisot wrote above (31 Marh 2016) this is a minor
issue. I believe the ms can be accepted.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#15 (comment)

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 5, 2016

@FedericoV Thanks. Please be sure to use the latest commit.

opetchey commented Apr 5, 2016

@FedericoV Thanks. Please be sure to use the latest commit.

@FedericoV

This comment has been minimized.

Show comment
Hide comment
@FedericoV

FedericoV Apr 5, 2016

The last commit reproduces beautifully. The report is also very well done - I don't use R at all, but I could follow almost all the steps/code.

I am satisfied with the paper in its current form. The only minor comments I have is that I would be curious to know how come the results for the Lyapunov coefficients was slightly different, and the exact procedure through which the zeros were removed.

Also - another comment. Is it possible to have within-document links? For example, when you talk about (red line in Fig. S1b) it would be handy to have a link that brought you to the relevant figure immediately.

The last commit reproduces beautifully. The report is also very well done - I don't use R at all, but I could follow almost all the steps/code.

I am satisfied with the paper in its current form. The only minor comments I have is that I would be curious to know how come the results for the Lyapunov coefficients was slightly different, and the exact procedure through which the zeros were removed.

Also - another comment. Is it possible to have within-document links? For example, when you talk about (red line in Fig. S1b) it would be handy to have a link that brought you to the relevant figure immediately.

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Apr 5, 2016

@FedericoV noted -- I think if @opetchey et al. have an idea, it can go in the ms. If not, since the differences are not really big, we can leave with this and move on to the next step.

As for the within-document links: maybe using \autoref in latex would work. But it can be fixed after we made the first decision.

tpoisot commented Apr 5, 2016

@FedericoV noted -- I think if @opetchey et al. have an idea, it can go in the ms. If not, since the differences are not really big, we can leave with this and move on to the next step.

As for the within-document links: maybe using \autoref in latex would work. But it can be fixed after we made the first decision.

@FedericoV

This comment has been minimized.

Show comment
Hide comment
@FedericoV

FedericoV Apr 5, 2016

Agreed completely.

On Tue, 5 Apr 2016 at 19:54 Timothée Poisot notifications@github.com
wrote:

@FedericoV https://github.com/FedericoV noted -- I think if @opetchey
https://github.com/opetchey et al. have an idea, it can go in the ms.
If not, since the differences are not really big, we can leave with this
and move on to the next step.

As for the within-document links: maybe using \autoref in latex would
work. But it can be fixed after we made the first decision.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#15 (comment)

Agreed completely.

On Tue, 5 Apr 2016 at 19:54 Timothée Poisot notifications@github.com
wrote:

@FedericoV https://github.com/FedericoV noted -- I think if @opetchey
https://github.com/opetchey et al. have an idea, it can go in the ms.
If not, since the differences are not really big, we can leave with this
and move on to the next step.

As for the within-document links: maybe using \autoref in latex would
work. But it can be fixed after we made the first decision.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#15 (comment)

@FedericoV

This comment has been minimized.

Show comment
Hide comment
@FedericoV

FedericoV Apr 13, 2016

@tpoisot I edited my first post here suggesting we accept this reproduction. It's very well done, the code is very readable. My suggestions are very minor and the reproduction is more than acceptable in its current form.

@tpoisot I edited my first post here suggesting we accept this reproduction. It's very well done, the code is very readable. My suggestions are very minor and the reproduction is more than acceptable in its current form.

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Apr 13, 2016

EDITOR

Accepted, April 13, 2016. Congratulations @opetchey et al. and thanks to @FedericoV and @yoavram

@rougier , what is the next step?

tpoisot commented Apr 13, 2016

EDITOR

Accepted, April 13, 2016. Congratulations @opetchey et al. and thanks to @FedericoV and @yoavram

@rougier , what is the next step?

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 13, 2016

Excellent! Many thanks!

Excellent! Many thanks!

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 14, 2016

Member

@tpoisot

Next step is the actual publication but the procedure is not really smooth yet. If you have some time to try it and tell what's not clear (new issue) that woudl be great ! Else I can do it.

The procedure is described at http://rescience.github.io/edit/ (see editing process). But mostly the goal is to freeze the repo (no more commit or only for obvious error), import it in the ReScience Archives (that will be the official repo) and modify the article such as to add editor/reviewers, change code/data repo links, set the volume (2) issue (1)

Once this done, you have to upload the archive to Zenodo (as journal article) and fill the the different fields (don't forget to add the editor field and volume/issue) and the link provided on the page above should make your upload part of the ReScience collection on Zenodo.

Last is to tell the original journal a replication has been published confirming the result, but I can do it.

Member

rougier commented Apr 14, 2016

@tpoisot

Next step is the actual publication but the procedure is not really smooth yet. If you have some time to try it and tell what's not clear (new issue) that woudl be great ! Else I can do it.

The procedure is described at http://rescience.github.io/edit/ (see editing process). But mostly the goal is to freeze the repo (no more commit or only for obvious error), import it in the ReScience Archives (that will be the official repo) and modify the article such as to add editor/reviewers, change code/data repo links, set the volume (2) issue (1)

Once this done, you have to upload the archive to Zenodo (as journal article) and fill the the different fields (don't forget to add the editor field and volume/issue) and the link provided on the page above should make your upload part of the ReScience collection on Zenodo.

Last is to tell the original journal a replication has been published confirming the result, but I can do it.

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 14, 2016

@tpoisot @rougier please let me know if I can or should help with any of these next steps.

@tpoisot @rougier please let me know if I can or should help with any of these next steps.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 14, 2016

Member

Nothing is required from you right now. But on the other hand, you can join the board as reviewer if you're interested. Just let me know.

Member

rougier commented Apr 14, 2016

Nothing is required from you right now. But on the other hand, you can join the board as reviewer if you're interested. Just let me know.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 19, 2016

Member

@tpoisot Do you want to handle the actual publication ?

Member

rougier commented Apr 19, 2016

@tpoisot Do you want to handle the actual publication ?

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 21, 2016

Member

I'll handle it.

Member

rougier commented Apr 21, 2016

I'll handle it.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 21, 2016

Member

@opetchey Can you give me the associated keywords ?

Member

rougier commented Apr 21, 2016

@opetchey Can you give me the associated keywords ?

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Apr 21, 2016

Yes, sorry about the delay -- was about to leave for a trip but a snow
storm happened...

Le jeu 21 avr 2016 à 5:31, Nicolas P. Rougier
notifications@github.com a écrit :

@opetchey Can you give me the associated keywords ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

tpoisot commented Apr 21, 2016

Yes, sorry about the delay -- was about to leave for a trip but a snow
storm happened...

Le jeu 21 avr 2016 à 5:31, Nicolas P. Rougier
notifications@github.com a écrit :

@opetchey Can you give me the associated keywords ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 21, 2016

Member

No problem. Does that mean you can handle the publication ?

Member

rougier commented Apr 21, 2016

No problem. Does that mean you can handle the publication ?

@tpoisot

This comment has been minimized.

Show comment
Hide comment
@tpoisot

tpoisot Apr 21, 2016

I'd rather let you do it if you can, I'll be sure to look at what the
different steps are.

Le jeu 21 avr 2016 à 11:29, Nicolas P. Rougier
notifications@github.com a écrit :

No problem. Does that mean you can handle the publication ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

tpoisot commented Apr 21, 2016

I'd rather let you do it if you can, I'll be sure to look at what the
different steps are.

Le jeu 21 avr 2016 à 11:29, Nicolas P. Rougier
notifications@github.com a écrit :

No problem. Does that mean you can handle the publication ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

@opetchey

This comment has been minimized.

Show comment
Hide comment
@opetchey

opetchey Apr 22, 2016

@rougier Yes, no problem. Keywords:
Ecology
Forecasting
Prediction
Chaos
Nonlinear dynamics
Plankton community
Species interactions

@rougier Yes, no problem. Keywords:
Ecology
Forecasting
Prediction
Chaos
Nonlinear dynamics
Plankton community
Species interactions

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 22, 2016

Member

This submission has been accepted for publication, and has been published and will soon appear at
http://rescience.github.io/read/

DOI

Member

rougier commented Apr 22, 2016

This submission has been accepted for publication, and has been published and will soon appear at
http://rescience.github.io/read/

DOI

@ReScience ReScience locked and limited conversation to collaborators Apr 22, 2016

@rougier rougier closed this Apr 22, 2016

@rougier rougier added the R label Jul 3, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.