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

implement wrappy pFBA using the normal L2 wrappers #773

Merged
merged 7 commits into from
Mar 5, 2023

Conversation

exaexa
Copy link
Collaborator

@exaexa exaexa commented Mar 3, 2023

No description provided.

@exaexa
Copy link
Collaborator Author

exaexa commented Mar 3, 2023

@stelmo I've got 1 remaining test fail on this, which is actually on ECModel (the result is like 89% instd of 91% of some enzyme). Could you pls check what might have gone wrong? Otherwise this is IMO gtg

@stelmo
Copy link
Collaborator

stelmo commented Mar 5, 2023

Will look now, could be the solver misbehaving though

Comment on lines +199 to +208
# TODO here we would normally also overload the matrix function, but that
# one will break once anyone touches variables of the models (which is
# common). We should have a macro like @model_does_not_modify_variable_set
# that adds the overloads. Or perhaps AbstractModelWrapperWithSameVariables?
#
# The same probably goes for other semantics;
# AbstractModelWrapperThatOnlyTouchesSemantics(...) ? (Which has an
# alternative in forcing people to overload all semantic functions in all
# cases of adding semantics, which might actually be the right way.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

@codecov
Copy link

codecov bot commented Mar 5, 2023

Codecov Report

Patch coverage: 72.72% and project coverage change: +0.61 🎉

Comparison is base (6a167a6) 88.02% compared to head (67bfb17) 88.63%.

Additional details and impacted files
@@               Coverage Diff                @@
##           mo-pfba-pipe     #773      +/-   ##
================================================
+ Coverage         88.02%   88.63%   +0.61%     
================================================
  Files                91       89       -2     
  Lines              2280     2262      -18     
================================================
- Hits               2007     2005       -2     
+ Misses              273      257      -16     
Impacted Files Coverage Δ
src/analysis/parsimonious_flux_balance_analysis.jl 80.00% <ø> (ø)
src/wrappers/SimplifiedEnzymeConstrainedModel.jl 78.57% <ø> (ø)
src/wrappers/MinimizeDistance.jl 57.69% <58.82%> (+26.44%) ⬆️
src/wrappers/EnzymeConstrainedModel.jl 93.02% <75.00%> (-1.85%) ⬇️
src/wrappers/MaxMinDrivingForceModel.jl 94.66% <75.00%> (-1.11%) ⬇️
src/types/accessors/ModelWrapper.jl 57.14% <100.00%> (ø)
src/types/accessors/bits/semantics.jl 100.00% <100.00%> (+27.45%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@exaexa exaexa merged commit a58407a into mo-pfba-pipe Mar 5, 2023
@exaexa exaexa deleted the mk-pfba-is-l2 branch March 5, 2023 18:27
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.

2 participants