-
Notifications
You must be signed in to change notification settings - Fork 30
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
SBML import: Alternative algorithm for identifying conservation laws #1748
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1748 +/- ##
===========================================
+ Coverage 77.72% 77.74% +0.02%
===========================================
Files 73 74 +1
Lines 11923 12012 +89
===========================================
+ Hits 9267 9339 +72
- Misses 2656 2673 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
python/amici/sbml_import.py
Outdated
if algorithm.lower() == "demartino": | ||
raw_cls = self._get_conservation_laws_demartino(ode_model) | ||
else: | ||
raw_cls = self._get_conservation_laws_new() |
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.
Ideas for better algorithm names?
I'd still keep the previous algorithm as an option, at least for another while.
python/amici/sbml_import.py
Outdated
coefficients),) | ||
return raw_cls | ||
|
||
def _get_conservation_laws_new( |
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.
def _get_conservation_laws_new( | |
def _get_conservation_laws_rref( |
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.
Might be somewhat confusing, as the other algorithm is also doing that as a first step, but I don't have a better idea 😅
# indices of retained species | ||
species_solver = list(range(ode_model.num_states_rdata())) | ||
|
||
algorithm = os.environ.get("AMICI_EXPERIMENTAL_SBML_NONCONST_CLS", "") |
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.
do we wan't to document this option? run demartino by default if rref fails?
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.
Yes, should be documented. I think I'd prefer having a closer look at any failures instead of an auto-fallback.
SonarCloud Quality Gate failed. |
... without Monte Carlo mumbo-jumbo.
Compute left nullspace of S via (numerical) Gaussian elimination. Going via SVD would be faster and numerically safer, but this seems to lead to weird coefficients.
For the model in
test_compute_moiety_conservation_laws_demartino2014
, this algorithm is slower than the one already implemented (~1min vs ~10s), but at least it's deterministic and not highly dependent on RNG seeds. For another model of similar size, the new algorithm takes ~2.5min instead of previously 1h20.