search

RuleWorld / bionetgen

Rule-based modeling framework
https://bionetgen.org/
MIT License
59 stars 25 forks source link

The scaling method is broken. #210

Closed LifeWorks closed 5 years ago

LifeWorks commented 5 years ago

Hi Jim,

Last week I try to modify a bit about the scaling method (HAS), but it keeps generating "Simulation method 'has' is not a valid option". I think the reimplementation of parsing the "Actions" block may be the reason. I located the changes related to it one commit each time. I suspect the implementation of simulation_protocol affected it.

But when I read the code, it seems fine to me. Would you mind check it? I checked the scaling method using the Model/heterogeneous_adaptive_scaling.bngl file proved.

Best!

Song

ASinanSaglam commented 5 years ago

Hi Song,

Correct me if I'm wrong but I suspect you are trying to run this with BioNetGen from your fork of the repo? Because I tested that file with our current distribution and it works without any issues for me.

I took a look at your fork of the repo I believe you only changed the Perl scripts that's calling "HAS" to PSA, however, that's not the only place where they are defined. In fact, you specifically need to change the C++ code (see run_network.cpp and related files in folder Network3) if you want to modify the HAS method. If you do this, you need to rebuild the code following the instructions on the main page of our repo.

Please let me know if there is something else I'm missing. I will close this issue in a few days unless there is another issue you have encountered that we can reproduce.

Edit: To be more precise, I tested that bngl file with the main distribution we currently have and it's working. I tried it with the Perl2 folder you have in your fork and got the same error. Can you provide us with a distribution of the BNG version you compiled and trying this with?

Edit 2: I did see the changes you made to the C++ files so I crossed that out, sorry about that. I'm still not sure if there is an error in the main repo though since I can't reproduce the issue you mentioned.

Best,

LifeWorks commented 5 years ago

I have changed the run_network accordingly. And it is actually not the problem of run_network.cpp.

I tested with the master branch. It is broken. The BioNetGen 2.4.0 is probably working.

ASinanSaglam commented 5 years ago

BioNetGen2.4.0 is built off of master branch of RuleWorld/bionetgen, that's why I'm not sure how to reproduce the issue you mentioned.

Edit: I just redownloaded a fresh 2.4.0 for Linux and tested it and it's working, just in case there was something wrong with my initial setup. Can you also tell me which OS you tested it with?

LifeWorks commented 5 years ago

I used linux. Also can you just git clone the current master branch rather than using BioNetGen 2.4.0? BNG2.4.0 is OK, the problem is happening with the master branch. If there is no problem when you using the current master branch then it is probably the problem of my side.

ASinanSaglam commented 5 years ago

Sure, I'll give that a try soon, latest Monday by the end of the day. Just to clarify though, the 2.4.0 is compiled from this commit which is very recent. So if there is an issue with a compilation from the current master branch, this means that the issue is very recently introduced (8-9 commits since January of this year) and we should be able to track the issue to one of those.

Either way, thank you very much for testing your changes out and helping out with testing the current repo, it is much appreciated!

Edit: Fixed commit link

ASinanSaglam commented 5 years ago

I git cloned a fresh copy, compiled it and ran the heterogeneous_adaptive_scaling.bngl test case and it's working fine on my end.

lh64 commented 5 years ago

Hi all, I pulled down @LifeWorks' fork, recompiled run_network, and was able to run partial_dynamical_scaling.bngl. I'm attaching the screen output here. There was one problem, an argument named popscale, which is getting a default value of 100 on the Perl side, is getting passed to run_network but it doesn't recognize it. It gets through that somehow though and finishes the run. So that needs to be fixed but it doesn't appear to be a fatal error so I'm not quite sure what the issue is. Just wanted to let you all know. test_psa_screen_output.txt

lh64 commented 5 years ago

Just a quick follow up. I looked in run_network.cpp and on line 279 the long option is called "poplevel". I assume changing it to "popscale" will solve the problem. Also, on line 309 there's no exit command after that error message that's getting thrown. I'll go ahead and fix that in the RuleWorld master and push it there.

LifeWorks commented 5 years ago

@lh64 Thanks for point it out. I will fix the problem and create the new pull request.

LifeWorks commented 5 years ago

I fixed the problem and created new pull request. Now it should right and working as expected.