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.
|
dweindl
changed the title
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.
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.
| def _get_conservation_laws_new( | |
| def _get_conservation_laws_rref( |
There was a problem hiding this comment.
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.
do we wan't to document this option? run demartino by default if rref fails?
There was a problem hiding this comment.
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.