-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: ModelAdapter pattern for organism specific parameters and customizations #168
Conversation
…KO code agnostic to which model is sent in (different species etc.) The idea is that this pattern could replace the "replacement of files"-strategy for running GECKO on a different species.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice demonstration of how the adapters are supposed to work. I think @IVANDOMENZAIN might have an opinion here if these adapters should go in the GECKO
repository or in ecModels
. Also, I'm not sure why getHumanGEMRootPath
needs to be defined outside the adapter.
So, to explain how you access the functions in the adapter: Example of how to call this: or just If you want to change some params in the adapter (which is why I left the params public, which is a bit ugly, but practical): GECKO here knows nothing about the specialized adapters, the modelAdapter sent in is just an object of a class that inherits the ModelAdapter base class. Let's look at Yeast8Adapter as an example - this class inherits modelAdapter. Since getSpontaneousRxns is abstract, YeastAdapter must implement that function - otherwise you will get an error when you try to create an object of that class. When the call is made, matlab will always use the function in the specialized class first, and GECKO doesn't have to know anything about that implementation in the specialized class - this could for example be placed in another repo. So, in java or c++, this is how you must solve it since those are typed languages. In matlab, you would technically not need a base class to accomplish this, as long as the object sent in has the right functions it will work anyway. But I think it is still better to use a base class - this makes it clear what needs to be implemented in the adapter and is what programmers would expect, and it also allows for default implementations in the base class (see getParameters for example). |
GeckoInstaller is persistent - it calls savepath, so if you restart matlab the path is still changed. The same could be added to RAVEN, but I think you have something there already perhaps? I always have these installer classes on my disk for Human-GEM, RAVEN and Yeast-GEM |
About where to place the specialized adapters: I'm thinking that during development of GECKO3 it is probably nice to have them in the GECKO repo, since we then don't need to change stuff in multiple repos and worry about having them synchronized. I do think it makes sense to then move them to another repo - my suggestion would be to place them in the GEM repos - so, the HumanGEMAdapter would go into the Human-GEM repo, etc. If someone makes a change, such as change how spontaneous reactions are stored in that repo, the adapter needs to be changed as well. One downside of that is that we cannot then (well, it would be weird) make YeastAdapter the default choice when calling functions, since GECKO will then not know about it. Technically it is still possible, but ... |
Ok, what confused me is when you run |
…lly didn't work. Also added an error message if there are semicolons in the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johan-gson I think the previous commit a27b88c was also meant to address that the separator should be :
instead of ;
, so I provided specific suggestions for this. Or maybe I just got confused?
Co-authored-by: Mihail Anton <[email protected]>
Co-authored-by: Mihail Anton <[email protected]>
Co-authored-by: Mihail Anton <[email protected]>
…ferent platforms have different separators, semicolon or colon, which messed things up.
Ok, I made a final change to GECKOInstaller - I asked matlab for the separator instead of hardcoding ';' - I actually think the previous code only worked on Windows... |
Great improvements @johan-gson! I am happy to approve the PR, but I think it would be useful to get a second pair of eyes to look at it before merging. |
Main improvements in this PR:
Added an adapter pattern that can enable us to use GECKO for different species without copying files. Right now the adapter is pretty small, but the idea is to add more functions in the base class when needed. The idea is then that an adapter is sent into GECKO when generating models, and that GECKO can ask the adapter for information that is accessed differently for each species/model. Examples include parameters, pathways to data, and which reactions are spontaneous. The code is not plugged in anywhere, so this is totally safe.
I also added an installation script, let's see what you think about that. To install (i.e., setup path):
cd to GECKO root
GECKOInstaller.install
to uninstall (i.e., remove GECKO from path):
GECKOInstaller.uninstall
This is practical if you have several versions of GECKO on your disk and want to switch between them.
I hereby confirm that I have:
devel
as a target branch (top left drop-down menu)