Skip to content
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

[Re] Chaos in a Three-Species Food Chain #15

Closed
gabrieldansereau opened this issue Feb 7, 2020 · 54 comments
Closed

[Re] Chaos in a Three-Species Food Chain #15

gabrieldansereau opened this issue Feb 7, 2020 · 54 comments

Comments

@rougier
Copy link
Member

rougier commented Feb 10, 2020

Thanks for your submission! The animation on the code README is great!

@karthik Can you edit this submission ?

@rougier
Copy link
Member

rougier commented Feb 12, 2020

@karthik Gentle reminder

@rougier
Copy link
Member

rougier commented Feb 13, 2020

@opetchey Could you edit this submission in Ecology (and become editor for ReScience C)? I can help you with the procedure.

@rougier
Copy link
Member

rougier commented Feb 21, 2020

Reminder: @opetchey Could you edit this submission in Ecology (and become editor for ReScience C)? I can help you with the procedure.

@rougier
Copy link
Member

rougier commented Feb 28, 2020

@dmcglinn Could you edit this submission in Ecology (and become editor for ReScience C)? I can help you with the whole procedure for this article.

@rougier
Copy link
Member

rougier commented Feb 28, 2020

@trashbirdecology Could you review this submission ?

@rougier
Copy link
Member

rougier commented Feb 28, 2020

@jsta Could you review this submission? (and eventually become an editor for ReScience since we have a lot of submission in ecology)

@trashbirdecology
Copy link
Member

I would be happy to review, but will likely not turn it around until mid to late March.

I am not familiar with Julia, so I will not be able to easily comment on the code, however, the code looks straightforward enough. Might want to ensure someone else can strictly review the code.

@rougier
Copy link
Member

rougier commented Feb 28, 2020

Thanks and mid or late March seems good to me.

@trashbirdecology
Copy link
Member

Could you please point me to the reviewer guidelines?

@rougier
Copy link
Member

rougier commented Feb 28, 2020

Oh yes, sorry, I forgot: https://rescience.github.io/edit/

@rougier
Copy link
Member

rougier commented Feb 28, 2020

Review are open such that you can read them at: https://github.com/ReScience/submissions/issues?utf8=%E2%9C%93&q=is%3Aissue

@rougier
Copy link
Member

rougier commented Mar 9, 2020

@ChrisRackauckas Could you review this submission? (reviewer guidelines at ttps://rescience.github.io/edit/)

@rougier rougier self-assigned this Mar 9, 2020
@ChrisRackauckas
Copy link
Member

I will not have the time to adequately devote to this, sorry.

@rougier
Copy link
Member

rougier commented Mar 10, 2020

@pboesu Could you review this submission? (reviewer guidelines at https://rescience.github.io/edit/)

@pboesu
Copy link
Member

pboesu commented Mar 13, 2020

Hi @rougier, happy to review this but turnaround will be slow - likely early to mid April.

@rougier
Copy link
Member

rougier commented Mar 13, 2020

I'm fine with this schedule. Also, would you be interested in becoming associate editor for ecology (after this review). We tend to have a fair number of submissions aroudn ecology and other editors are quite busy (or are themselves authors like @tpoisot).

@trashbirdecology
Copy link
Member

Hoping to get to this this week, @rougier and @gabrieldansereau

@rougier
Copy link
Member

rougier commented Apr 13, 2020

@pboesu @trashbirdecology Gentle reminder.

1 similar comment
@rougier
Copy link
Member

rougier commented Jul 6, 2020

@pboesu @trashbirdecology Gentle reminder.

@rougier
Copy link
Member

rougier commented Aug 4, 2020

@karthik Could your eview this submission and/or suggest reviewers since @pboesu and @trashbirdecology have become unresponsive ?

@trashbirdecology
Copy link
Member

trashbirdecology commented Aug 4, 2020

So sorry to all involved -- especially the authors -- I dropped the ball here.

After looking back at my notes from the winter, I was/am struggling with self-learning Julia enough to give a proper review of this reproduction. I don't think I will be able to find the time to do so in the coming weeks, either. I am deeply sorry for this huge inconvenience I have caused the authors and editor.

@karthik
Copy link
Member

karthik commented Aug 4, 2020

Hi @rougier I don't have time as I'm leaving for vacation soon. But I'd recommend Carl Boettiger (cboettig).

@tpoisot
Copy link

tpoisot commented Aug 8, 2020

Hi @trashbirdecology @karthik @rougier - it's all good -- we're living in the middle of a pandemic and trying to do whatever we can. No balls dropped by anyone as far as I'm concerned, these are just the times we live through. There'll be more reviewers at some point, meanwhile, take care.

@rougier
Copy link
Member

rougier commented Aug 10, 2020

@karthik Thanks for the pointer
@tpoisot thanks for your patience

@cboettig
Copy link

Final remarks:

Overall the Julia code looks very clean and easy to read. Given that I seem to still get the approximately the same desired figures out, (unless there's some intermediate cache I didn't notice? the above errors/warnings are annoying but minor issues. Unless the authors or reviewers decide otherwise, I'm happy to leave it to them on how best (if at all) to address the issues I've raised above before final acceptance, and don't think I need to re-review any changes. I enjoyed this piece and am happy to recommend it for publication.

@gabrieldansereau
Copy link
Author

Thank you for your review @cboettig. I am very happy you could get up and running in Julia so quickly.

I will address your comments with some changes as soon as possible. But briefly:

  • You raise good points on usability (runtime & needs, how to run the script). I'll add more info to clarify
  • Interesting point on the choice of integration algorithm -- I'll re-explore and report back on it.
  • The errors and warnings are expected and negligible, as you suspected (I had the same). The first is a warning about the integration itself, and I don't think there's much to do about it. The second is a graphics display error I sometimes get too on Ubuntu, but the figures are produced correctly anyways. The last one occurs when precompiling the packages -- again I don't think there's much to do. I'll check again if I can do something, else I'll add some comments to document.
  • Thanks for noting the sensitivity typo and the figures' grainy look. I'll fix them and proof-read again.

@gabrieldansereau
Copy link
Author

Here are the changes I made to address @cboettig's comments (@rougier I guess you're acting as editor?). The full diff is here (I've also made some minor changes to make a few things clearer).

  • 7b0e993 - Clarifies how to run the scripts
  • 6152ff5 - Clarifies the runtime & memory needs
  • e2ed990 - Fixes the sensitivity typo
  • 6561093 - Replaces the figures by a sharper vectorized version
  • f2b0b8b - Fixes the 3rd warning Warning: Symbolic calculations could not initiate
  • 523accc - Adds a note on how to fix the 2nd warning GKS: Open failed in routine OPEN_WS. The figures should reproduce despite it. I believe it comes from external libraries and could be fixed on the user-side by installing external dependencies, but I cannot test it.
  • 21b32b - Adds a note to explain that the 1st warning There is either an error in your model or the true solution is unstable is expected. In the context of chaotic behaviour, we believed it made sense for the model not to converge and to be unstable for a few of the higher values tested for the b1 parameter. I see how it can be annoying as it's repeated many times, but I think it's better to leave it like that to warn potential users, rather than to find a way to hide it.

Regarding the choice of numerical integration algorithm, I've added a small note to the text in ae53d7s. Selecting a specific algorithm had an impact but did result in a similar behaviour for the system -- in fact, we had to use RK4 instead of the automatic solver to replicate some figures correctly (but it had its downsides too). Not knowing the original algorithm and initial conditions, we thought it best to start from the automatic algorithm if possible, then fiddle with the initials conditions to find the closest reproduction. So the initial conditions would not necessarily be the same with another algorithm.

@rougier
Copy link
Member

rougier commented Aug 17, 2020

@cboettig Thanks for the review.
@gabrieldansereau Thanks for such timely correction. I'm still looking for the second reviewer.

@rougier
Copy link
Member

rougier commented Aug 17, 2020

@ha0ye
Copy link

ha0ye commented Aug 17, 2020

This is very much in my wheelhouse, but my most optimistic projection for time is October (probably later).

*checks submission date*

¯\_(ツ)_/¯

@rougier
Copy link
Member

rougier commented Aug 17, 2020

@ha0ye Thanks for quick answer. Since we're aleady quite late with this submission, I'll wait for other answer first.

@pboesu
Copy link
Member

pboesu commented Aug 17, 2020

My sincere apologies for letting this fall by the wayside (and thanks to @aammd and @rougier for chasing me). I'm now back from leave and will have a look at the revised submission now.

@ChrisRackauckas
Copy link
Member

I do not think I will have adequate time to devote to this right now.

@pboesu
Copy link
Member

pboesu commented Aug 18, 2020

@rougier @gabrieldansereau
I agree with @cboettig . This is an excellent concise replication. I only use julia very sporadically and had to install it from scratch on the machine used to review the code (Windows 8.1), but it was easy to run the code and reproduce the figures.

In addition to the warnings already discussed (and addressed by the authors) julia triggered a Windows Firewall warning about "gksqt.exe", possibly an analogue to the ubuntu GKS warning. Additionally there were compilation warnings for DiffEqBase:

WARNING: Method definition combine_axes(Any, Any) in module Broadcast at broadcast.jl:484 overwritten in module DiffEqBase at C:\Users\Philipp\.julia\packages\DiffEqBase\gTsoC\src\diffeqfastbc.jl:18.
  ** incremental compilation may be fatally broken for this module **

and when running the code in figure4.jl there were warnings about variable names:

┌ Warning: Assignment to `b1` in soft scope is ambiguous because a global variable by the same name exists: `b1` will be treated as a new local. Disambiguate by using `local b1` to suppress this warning or `global b1` to assign to the existing global variable.
└ @ C:\Users\Philipp\Documents\bitbucket\ReScience\2019_replication_HastingsPowell_1991\code\figure4.jl:22
┌ Warning: Assignment to `prob` in soft scope is ambiguous because a global variable by the same name exists: `prob` will be treated as a new local. Disambiguate by using `local prob` to suppress this warning or `global prob` to assign to the existing global variable.
└ @ C:\Users\Philipp\Documents\bitbucket\ReScience\2019_replication_HastingsPowell_1991\code\figure4.jl:24
┌ Warning: Assignment to `sol` in soft scope is ambiguous because a global variable by the same name exists: `sol` will be treated as a new local. Disambiguate by using `local sol` to suppress this warning or `global sol` to assign to the existing global variable.

Otherwise I only have a handfull of minor comments about the write-up and figures. I've created a pull request to the original repository that implements these suggestions: BIO6032/2019_replication_HastingsPowell_1991#23

BIO6032/2019_replication_HastingsPowell_1991@ef8143f fixes a typo in the axis label of Fig. 5

BIO6032/2019_replication_HastingsPowell_1991@4fbc7ae is a suggestion to use LaTeXStrings for proper subscripts in Figure labels. This may require some minor refactoring to shift the new dependency to main.jl I didn't commit the changed *.toml files as I am unsure how to work with these. (Also this is a rather pedantic suggestion tbh - so I leave it up to the authors whether or not to incorporate this.)

BIO6032/2019_replication_HastingsPowell_1991@5013f87 includes comments and minor edits to the manuscript. A few comments that need additional revision by the authors are prefixed with a LaTeX comment string %Reviewer comment:

I believe this manuscript is in good shape to be published once these minor changes are implemented.

@gabrieldansereau
Copy link
Author

Thank you very much for your review @pboesu.

I believe the soft scope warning came in more recent versions of Julia. Is it possible you're using 1.5 instead of 1.3.1?

Thanks for adding your comments in a pull-request. I am on vacation this week, so maybe one of my co-authors wants to handle it? @FrancisBanville @aammd @tpoisot
Otherwise I'll be happy to do it next week.

@rougier
Copy link
Member

rougier commented Aug 21, 2020

@pboesu Thank you very much for your review.
@gabrieldansereau Your corrections can wait until next week.

@rougier
Copy link
Member

rougier commented Aug 21, 2020

@ChrisRackauckas @ha0ye Thanks for your answers, I've finally found the second reviewer.

@gabrieldansereau
Copy link
Author

gabrieldansereau commented Aug 31, 2020

@rougier Sorry for the delay.

Thanks again for your review @pboesu

We merged your pull-request and added these changes:

I think the "gksqt.exe" warning is indeed similar to the Ubuntu GKS one. I'd leave it as is since it's user-specific and does not stop reproduction. Same for the DiffEqBase precompilation warning, which does not seem to impact either.

@rougier
Copy link
Member

rougier commented Sep 1, 2020

Nice. @cboettig @pboesu Are tyou satisfied with the corrections and answers provided by @gabrieldansereau. If yes (you can add a thumb up), we can proceed with publication.

@cboettig
Copy link

cboettig commented Sep 1, 2020 via email

@pboesu
Copy link
Member

pboesu commented Sep 1, 2020

@rougier Happy to see this published!

@gabrieldansereau
Copy link
Author

@rougier Hope it's ok -- I went ahead and updated the metadata.yaml file. I added the DOI for the code, the review URL, and the reviewers' info BIO6032/2019_replication_HastingsPowell_1991@3a46622

@rougier
Copy link
Member

rougier commented Sep 8, 2020

@gabrieldansereau That's perfect. Let check for the volume/issue and I will send you the DOI (or make a PR on your repo)

@rougier
Copy link
Member

rougier commented Sep 10, 2020

I've upload an edited version at https://sandbox.zenodo.org/record/668390. This is the Zenodo sandbox (entry will dissappear in one day). Can you check everything's fine ? If yes, I'll publish on the actual server

@rougier
Copy link
Member

rougier commented Sep 10, 2020

@gabrieldansereau Forgot to tag you

@gabrieldansereau
Copy link
Author

@rougier yes everything is fine, thanks!

@rougier
Copy link
Member

rougier commented Sep 10, 2020

Thansk, I'll publish it then.

@rougier
Copy link
Member

rougier commented Sep 10, 2020

It's published at https://zenodo.org/record/4022518 and will appear on the ReScience website in a few minutes. Thanks again for your submission and congratulation again ! I've made a PR with publication data on your repo.

By the way, I forgot to ask because you were too fast in updating the metadata but now the preferred and recommended way to save your code is to use software heritage (https://www.softwareheritage.org/save-and-reference-research-software/).

@gabrieldansereau
Copy link
Author

Great, many thanks to you! I've merged your PR on our repo.

I did not know about Software Heritage, I'll check it out!

@rougier rougier closed this as completed Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants