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

Introduction of optimizations originally developed in Gecko Light. #154

Merged
merged 1 commit into from
Nov 30, 2021

Conversation

johan-gson
Copy link
Collaborator

@johan-gson johan-gson commented Nov 2, 2021

…e optimizations concern getEnzymeCodes and matchKcats. The findInDBOld is still needed for KEGG matches, I haven't looked at that.

Some new optimizations were also introduced since there were other bottlenecks for yeast. I use containers.Map which may require a somewhat new matlab version.

Main improvements in this PR:

This is a continuation of a series of flawed pull requests, but I think this should be right.

Cheewin has done most of the testing, I have run some simple tests, where it produced identical results as before.

In short, there are many small optimization steps introduced in this PR, originally developed in GeckoLight, that speeds up getEnzymeCodes and matchKcats with roughly a factor 10. The most advanced ones rely on pregenerating indexes for faster search, which speeds things up considerably in the main loops. I also introduced some additional optimizations not present in GeckoLight that turned out to be important for yeast. I used containers.Map which may require a somewhat new matlab version.

I hereby confirm that I have:

  • Tested my code with all requirements for running GECKO
  • Selected devel as a target branch (top left drop-down menu)

…e optimizations concern getEnzymeCodes and matchKcats. The findInDBOld is still needed for KEGG matches, I haven't looked at that.

Some new optimizations were also introduced since there were other bottlenecks for yeast. I use containers.Map which may require a somewhat new matlab version.
@mihai-sysbio
Copy link
Member

This is a continuation of a series of flawed pull requests, but I think this should be right.

No worries, I'm glad to see this advancing.

Cheewin has done most of the testing, I have run some simple tests, where it produced identical results as before.

It would be nice to hear more about the testing @ckitti (especially if done for different setups).

In short, there are many small optimization steps introduced in this PR, originally developed in GeckoLight, that speeds up getEnzymeCodes and matchKcats with roughly a factor 10. The most advanced ones rely on pregenerating indexes for faster search, which speeds things up considerably in the main loops. I also introduced some additional optimizations not present in GeckoLight that turned out to be important for yeast.

That sounds amazing 🚀

I used containers.Map which may require a somewhat new matlab version.

Can you specify which?

@johan-gson
Copy link
Collaborator Author

So, we investigated if the version would be a problem some months ago, and concluded it was not a large problem. I don't remember exactly which version, it is not super easy to figure it out, I spent some time on it last time.

@mihai-sysbio
Copy link
Member

mihai-sysbio commented Nov 2, 2021

So, we investigated if the version would be a problem some months ago, and concluded it was not a large problem. I don't remember exactly which version, it is not super easy to figure it out, I spent some time on it last time.

According to the documentation, containers.Map has been "Introduced in R2008b". It's surprising that this would have created problems. Is there a chance that there could be something else?

@johan-gson
Copy link
Collaborator Author

Hmm, ok, no, this is what I used. Not much of a problem then.

@edkerk
Copy link
Member

edkerk commented Nov 2, 2021

FYI, RAVEN also uses containers.Map in various functions, and has done so "forever" (at least 2016), I've never heard of anybody running into problems with it.

Cheewin has constructed an ec-model with this PR and the previous code, and got identical models.

@johan-gson
Copy link
Collaborator Author

Ok, I take it he will make some kind of review here, although it has already been reviewed, so I can finally merge this...

@edkerk edkerk self-requested a review November 30, 2021 15:49
@edkerk edkerk merged commit 90cc6ef into devel Nov 30, 2021
@edkerk edkerk deleted the optimizationsKcats branch November 30, 2021 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants