ReScience / submissions

ReScience C submissions
28 stars 7 forks source link

[Re] A Multi-Functional Synthetic Gene Network #13

Closed baioc closed 2 years ago

baioc commented 4 years ago

Original article: A Multi-Functional Synthetic Gene Network: A Frequency Multiplier, Oscillator and Switch

PDF URL: [Re] A Multi-Functional Synthetic Gene Network Metadata URL: metadata.yaml Code URL: GitHub repository

Scientific domain: Biological Computing / Synthetic Biology Programming language: Octave Suggested editor: C. Titus Brown, Konrad Hinsen

rougier commented 4 years ago

Thank you for your submission and sorry for the late answer. An editor will be soon assigned. @khinsen @ctb Can you edit this sublmission?

ctb commented 4 years ago

I don't have time until April I'm afraid :(

On Tue, Jan 21, 2020 at 09:59:23PM -0800, Nicolas P. Rougier wrote:

Thank you for your submission and sorry for the late answer. An editor will be soon assigned. @khinsen @ctb Can you edit this sublmission?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/ReScience/submissions/issues/13#issuecomment-577021706 -- C. Titus Brown, ctbrown@ucdavis.edu

rougier commented 4 years ago

@khinsen, can you edit it?

khinsen commented 4 years ago

I'll do my best!

@aaronshifman and @degoldschmidt, can you review this submision?

degoldschmidt commented 4 years ago

@khinsen Thank you for inviting me to review, but unfortunately I have to decline, because the topic is out of my range.

khinsen commented 4 years ago

Thanks for your quick reply @degoldschmidt !

@thmosqueiro, can you review this submission?

khinsen commented 4 years ago

🛎Gentle reminder: @aaronshifman and @thmosqueiro, could you please let me know if you can review this submission?

aaronshifman commented 4 years ago

My apologies @khinsen I missed the last email. This looks like some very cool work. I'd be happy to review.

khinsen commented 4 years ago

Thanks @aaronshifman for accepting to review this paper! I will continue to look for a second reviewer, but you can start reviewing right now if you want.

khinsen commented 4 years ago

@Kevin-Mattheus-Moerman Can you review this submission?

Kevin-Mattheus-Moerman commented 4 years ago

@khinsen sorry this is not my area of expertise

aaronshifman commented 4 years ago

@khinsen Sure thing, I'll try to get to it within the next week.

rougier commented 4 years ago

Gentle reminder

aaronshifman commented 4 years ago

@khinsen appologies for the delay. @baioc thank you for the replication of this interesting article. It would appear that you've successfully replicated most of the original implementation with the exception to mainly your figure 3 which you address in the paper.

Code Comments

This will not be a full review as at this stage the code is insufficient for publication. It would seem that you've done an excellent job trying to make each file both stand-alone and fairly well documented. The level of similarity between each simulation is incredibly high and as such it is very difficult to see where things differ between simulations and additionally what is simulation specific and what is general (i.e. euler stepping).

I will try to point places where this could be easily improved.

Firstly parameters. There are some parameters that are constant across all simulations (as far as I can tell). For example mRNA degradation rates are 2.5e-3. This could be refactored into a common location (or config file if you should so choose). Other parameters have a common value except in some simulations when they change i.e. in reduced simulations these could take default values and then update where necissary.

Secondly is the euler stepping. When trying to understand what a simulation is doing it is challenging in that I have to parse code that after reading I understand to be "unimportant" as its just the FE algorithm. I understand that you have a few different kinds of simulations with repeated pulses or single pulses. However most of this could be refactored into a few functions and instead of reading

Algorithm step 1
Algorithm step 2
.
.
.
Alogrithm step n

I could read

solution = fe(parameters)

and everything becomes much clearer.

Lastly is the fact that if I want to run your code I have no ability to just run everything. I have to run each file individually, and then move the saved figure to a common location for your pdf. I would be nice is there was at least one script to run everything and if they all saved figures into a common location.

This is not an exercise in "writing good code" but an important step. As being able to abstract the simulation into the "science" and the "implementation" allows for easier understanding of the simulation and the methods behind it as well as "debugging" in the case of a differing simulation from the reference.

n.b. For stochastic simulations FE in inappropriate due to noise scaling. See Euler-Maruyama alogrithm for a stochastic adaptation. This could be in part the challenge with your noise scaling in the stochastic replication. The authors do not mention how they performed this in their supplemental. However if they did use ode45 the adaptive tolerances will bias noise towards a lower amplitude.

Implementation Comments

It would seem that most figures were well replicated. You've addressed the differences in your figure 3/ their figure 7. However I've found a few other discrepancies

Article Comments

Overall I found the paper well organized and easy to read. My only large comment is that to get model equations I have to refer to the original manuscript. These should be included in the replicated manuscript.

Some minor comments

baioc commented 4 years ago

@aaronshifman thank you for the kind review.

As you have pointed out, our intention was to make each script a standalone experiment, but I do agree that a more maintainable approach would be to configure each simulation individually in a separate file. We shall also look into the Euler-Maruyama algorithm for our stochastic simulations and after refactoring it should be easier to adjust the mentioned discrepancies in our figures, as well as to provide a script to run all experiments and generate figures accordingly.

Regarding the article, do you suggest we include the model's full ODE system, the QSSA approximation equations, or both?

aaronshifman commented 4 years ago

My inclination would be to include both, however since the full system is quite large if there is not a good way to present the whole system and the QSSA approximation then I would suggest including the approximation and a description (in text) of the full model.

khinsen commented 4 years ago

Thanks @aaronshifman for your detailed review, and thanks @baioc for your constructive first reaction. And apologies to both of you for being late (I am close to declaring GitHub notification bankruptcy). Finally, I am aware that I still need to find a second reviewer!

khinsen commented 4 years ago

@ruimcportela Can you review this submission? It's Octave, not Matlab, but that's perhaps close enough for you.

rougier commented 4 years ago

Since we're close to April, maybe @ctb is available now.

baioc commented 4 years ago

Apologies for the delay. The replication repository's master branch has been updated to acknowledge reviewer comments and I believe most issues have been addressed.

The exception being the numerical integration method used in stochastic experiments, which remain the same. This was done to in order to preserve the Chemical Langevin Equations as defined in the original article's Supporting Information File S6.

EDIT: just noticed Figure 8's "data1" label, this has been removed in the latest commit.

rougier commented 4 years ago

@ReScience/reviewers We would need a second reviewer for this replication in Biological Computing / Synthetic Biology using octave.

aaronshifman commented 4 years ago

Sorry for the delay @khinsen and @baioc the revisions slipped by me.

I'll try to get a review back early next week. Just a cursory look would suggest @baioc has done a good first pass at refactoring the codebase. I do not understand your comment about preserving the Langevin Equations. If done properly the choice of integration method will not change the "meaning" of the differential equation. Using a standard Forward Euler will in effect scale the noise amplitude. (See here for a hand-waving justification).

I'll upload a formal review ASAP.

rougier commented 4 years ago

@khinsen Any progress?

baioc commented 3 years ago

Greetings,

I was recently reminded of this ongoing submission and since it is now a little more than 1 year old, I wonder if I should consider it as rejected (and close the issue) or keep it open in case this is how long it usually takes.

@rougier I hope you don't mind that I tag you to make sure someone gets notified.

rougier commented 3 years ago

@baioc So sorry for being so late in the review. @khinsen Did you find second reviewer ? @baioc @ctb Would you have some bame to suggest for the second reviewer ?

baioc commented 3 years ago

Would you have some bame to suggest for the second reviewer ?

Perhaps @thmosqueiro or @costashatz ?

costashatz commented 3 years ago

@baioc thank you for the invitation. Unfortunately, this is a busy period for me and I will not be able to review this submission. Thanks again and sorry for not being able to help..

rougier commented 3 years ago

@khinsen Gentle reminder

khinsen commented 3 years ago

Thanks for the reminder @rougier!

I had contacted two potential reviewers by mail back in September, but never got a reply. I will try a second time.

khinsen commented 3 years ago

Two quick replies.. but negative.

In order to move on with this, I propose a slight deviation from the usual ReScience process, if @oliviaguest, @benoit-girard, and @rougier don't have any objections: I auto-proclaim myself a reviewer for this paper.

My knowledge of synthetic biology is certainly weak, but it's only the computational aspects that matter for this replication, and they are about numerically solving a system of ordinary differential equations, which is something I have plenty of experience with. And I don't mind exploring the current state of Octave at the occasion, a piece of software that I haven't used for about 15 years.

rougier commented 3 years ago

Actually I think we wrote that an editor can also be a reviewer such that it is not even a deviation.

khinsen commented 3 years ago

It's hard to be innovative in such a forward-looking organization :-(

khinsen commented 3 years ago

@baioc Here comes my review of your article and code, in the current version that has already been improved based on the first review by @aaronshifman.

The submitted article attempts to replicate all of the numerical simulations in the original paper, using slightly different numerical techniques (a different ODE solver), and a new implementation written in Octave. In addition to replicating the simulation of the original work based on the quasi-steady state approximation (QSSA), the authors also perform some simulations using the more elaborate initial system of ODEs. Presumably that was not (or less) feasible ten years ago, when the original study was published. The assessment of QSSA as part of the replication adds to the interest of the replication article.

Most but not all of the the replication results agree very well with the figures shown in the original study. The biggest difference is observable in the bifurcation time scales of Fig. 3. The authors note the difference but provide no explanation, which is understandable given that the original source code is not available. There are also differences in the results of the stochastic simulations (described in supplementary material to the original work), where the authors had to modify the parameters quoted in the original work in order to get reasonably similar results.

The Octave code provided by the authors is not very complicated and very readable. I was able to run it code following the instructions, using the current version of Octave (6.2.0, running under macOS) rather than the version 5.1.0 used by the authors. For the benefit of long-term reproducibility, the authors should also mention the version numbers of the two add-on packages ("control" and "signal") that were used. For my evaluation, I used control 3.2.0 and signal 1.4.1.

Installation of "control" issued the error message "warning: LFLAGS is deprecated and will be removed in a future version of Octave, use LDFLAGS instead", which suggests that something may break in the future.

The figures that I obtained on my computers look very similar to those in the submitted paper. The only visible differences concern font sizes and line styles.

baioc commented 3 years ago

Thank you @khinsen for your review.

W.r.t. the additional Octave packages: the versions which were used are mentioned in the README.md file (I reckon there is some rephrasing that could be done to make that information more clear). Do you mean we should mention it directly in the article?

khinsen commented 3 years ago

@baioc My preference would be to mention all version numbers in the article, in particular if there are only three of them.

But I did have another look at the README.md file as well, and noticed a discreancy: the article says Octave 5.1.0, the README says 5.2.0.

I also wonder if Octave actually provides a way to install specific versions of dependencies. I couldn't find any information on this, but I didn't spend that much time searching for it either. If it is possible and not too complicated, it would be nice to reformulate the instructions in README.md accordingly.

baioc commented 3 years ago

I believe the most recent commits to the dev branch address these comments: package versions are now mentioned in the replication paper and install instructions in the README now fetch, from Octave Forge, the specific versions which were used.

I also re-ran all experiments in Octave 6.2.0 (under Manjaro-Linux) and can confirm they still work, with only minor stylistic changes to plots, as reported by @khinsen.

khinsen commented 3 years ago

Thanks @baioc, this looks very good!

@aaronshifman Are you happy with this version as well, so we can accept it?

aaronshifman commented 3 years ago

Sorry in git notification hell so I missed this. I'll check out the updates today and try go give a verdict in the next day or so.

aaronshifman commented 3 years ago

Hello @baioc thank you for the updates to your codebase along with the manuscript.

I believe that the codebase is now more readable and packages the experiments nicely, and faithfully reproduces the original manuscript, with the exceptions that that authors address in Figure 3. As @khinsen mentioned the reference work was done a long time ago and certainly I'm sure tolerances due to lack of computation power would be quite lax. That said, it would be nice to implement this system in a bifurcation suite (XPPAUT or MATCONT) and confirm the new results (or re-running the simulation with a smaller time step).

My only hanging issue is the stochastic simulations. The use of the FE algorithm will incorrectly scale the noise by \sqrt(dt)... however since the original authors used the adaptive ode45 in MATLAB (also not an ideal method but for a different reason - noise down scaling during fast transients). I thought this replication could serve as an inference as to the effective noise-scale in the original implementation. However, since the noise scale in both cases are in a sense "incorrect", but in different ways. I would defer this decision to whether this should be corrected to the editor.

Other than these two comments, I feel like this article is suitable for publication.

khinsen commented 3 years ago

Thanks @aaronshifman for your comments!

From my point of view as a ReScience editor (rather than a domain specialist), the first proposition (improving bifurcation analysis) is certainly of interest but also well beyond doing a replication. If @baioc wants to do this as an added-value section, that's fine with me, but it's not a requirement.

The second point is more relevant from a replication point of view. The replication uses a different integration algorithm compared to the original work, and yields different noise patterns for the stochastic simulations. The minimal requirement is discussing this issue in the replication paper, which @baioc did. The best next step from a purely "replication" point of view would be to use ode45 for the stochastic simulations and see if the noise patterns are replicable. This could well fail because of different ode45 implementations in Matlab vs. Octave.

Beyond such a check, one can aim aim for insight into what is happening, which is what @aaronshifman suggests. As for his first point, this is a useful addition but not not a requirement for ReScience. We are dealing with suboptimal practices here: using ODE solvers for SDE is fundamentally wrong, but also common practice in many fields. My personal preference for shedding light on the impact of this mistake would be the use of a proper SDE integrator. But that could well turn out to become a research project of its own, depending on the state of the art in this particular type of equations, which I am not familiar with.

@baioc What's your point of view on this?

baioc commented 3 years ago

First I would like to thank @aaronshifman and @khinsen for the reviews, and apologize for the delay on the reply. Since this has been going on for well over a year, I would prefer not to go over significant changes to the implementation and leave using an SDE aside since I'm not familiar with those at all.

That said, if running the stochastic simulations using Octave's ode45 doesn't prove being much of a problem, I will compare it to the FE method and see if I can reproduce the oscillator mode irregularities. Also with respect to the noise scaling in FE, this is somewhat addressed by the downscaling mentioned in section 3.5, but I can definitely try to do a better job at explaining how that is caused by the integration method being used.

As for the bifurcation results, I will run some more simulations varying the timestep (which is something that may have been already done earlier last year, so I don't think the results are going to be different) and mention those in the text as well.

However, I am quite busy this month, so expect this to be done only by the end of July.

khinsen commented 3 years ago

Thanks @baioc for your comments, and for your patience with this review process! Take your time and let us know when you are done.

khinsen commented 2 years ago

@baioc Any update?

baioc commented 2 years ago

@khinsen apologies for being late. It turns out that my prediction of when I'd have the time to tackle this was way off (it also doesn't help that my studies and work have moved far from the subject of this paper). Now that we're approaching the end of the year, however, I'll soon have some free time to work on this. I'll let you know once the repository is updated with the additional simulations and the accompanying commentary is added to the article text.

baioc commented 2 years ago

Alright, I added an extra simulation configuration for the integration method to be used and was able to reproduce all experiments using Octave's ode45 solver. Results match those obtained using FE, including in the stochastic simulations (we still needed to compensate noise variance) and the same oscillator irregularities could be observed.

With respect to the period-doubling bifurcation, varying the Euler time step between 1 and 60 (or using the adaptive integrator) showed no significant difference that could explain the huge offset between the bifurcation's location in the reproduction and in the original work. The possible explanation for this remains the same as before: there could have been some mismatch in the parameters which were actually used in the original paper.

The article text was updated to take those additional experiments into consideration and the repository's main branch has been updated as well. Hopefully these changes address the remaining comments that came up in the review.

khinsen commented 2 years ago

Thanks @baioc for the updated article and code! To me this looks very good.

@aaronshifman Any comments from your side?

rougier commented 2 years ago

Any progress ?

khinsen commented 2 years ago

:bell: @aaronshifman Any comments from your side on this revision?

aaronshifman commented 2 years ago

@khinsen Apologies for the delay, after looking at the revisions made by @baioc, I believe they've adequately addressed my concerns. I have no further issues with the state or the article or codebase.

khinsen commented 2 years ago

Thanks @aaronshifman !

So... I think we can... finally... proclaim this submission accepted!

I will handle the publishing step tomorrow. @baioc, I may well request help from you in producing the final manuscript, depending on how things turn out.