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

Make kwargs for convenience enzyme constrained consistent #759

Merged
merged 3 commits into from
Feb 18, 2023

Conversation

stelmo
Copy link
Collaborator

@stelmo stelmo commented Feb 17, 2023

total_enzyme_capacity is used in the simplified enzyme constrained model

@stelmo stelmo requested a review from exaexa February 17, 2023 15:34
@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

not sure why the tests did not run

Copy link
Collaborator

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

1 note otherwise ok

@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

no idea why this is giving an error

[ Info: Precompiling COBREXA [babc4406-5200-4a30-9033-bf5ae714c842]
WARNING: Method definition make_enzyme_constrained_model(COBREXA.Types.AbstractMetabolicModel) in module Reconstruction at C:\Users\stelmo\Documents\GitHub\pkgs\COBREXA.jl\src\reconstruction\enzyme_constrained.jl:39 overwritten at C:\Users\stelmo\Documents\GitHub\pkgs\COBREXA.jl\src\reconstruction\enzyme_constrained.jl:179.
  ** incremental compilation may be fatally broken for this module **

WARNING: Method definition kwcall(Any, typeof(COBREXA.Reconstruction.make_enzyme_constrained_model), COBREXA.Types.AbstractMetabolicModel) in module Reconstruction at C:\Users\stelmo\Documents\GitHub\pkgs\COBREXA.jl\src\reconstruction\enzyme_constrained.jl:39 overwritten at C:\Users\stelmo\Documents\GitHub\pkgs\COBREXA.jl\src\reconstruction\enzyme_constrained.jl:179.
  ** incremental compilation may be fatally broken for this module **
┌ Warning: Replacing docs for `COBREXA.Reconstruction.make_enzyme_constrained_model :: Tuple{COBREXA.Types.AbstractMetabolicModel}` in module `COBREXA.Reconstruction`
└ @ Base.Docs docs\Docs.jl:243

@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

Ahhh, you can't dispatch on kwargs!

@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

Sigh, this means that this PR won't work 😢

@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch from 98f2891 to afcb691 Compare February 17, 2023 18:16
@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

Fixed this!

@stelmo
Copy link
Collaborator Author

stelmo commented Feb 17, 2023

also changed the with pipe to make it more general for simplified enzyme models

@stelmo stelmo added do not merge When a PR is labelled as such, do not merge and removed do not merge When a PR is labelled as such, do not merge labels Feb 17, 2023
@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch from 8a62d49 to dbc5282 Compare February 17, 2023 22:03
@stelmo stelmo requested a review from exaexa February 17, 2023 22:04
Copy link
Collaborator

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

I added 2 suggestions re argument handling, otherwise OK

@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Base: 88.19% // Head: 88.22% // Increases project coverage by +0.02% 🎉

Coverage data is based on head (68f46f3) compared to base (2e1fc07).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #759      +/-   ##
==========================================
+ Coverage   88.19%   88.22%   +0.02%     
==========================================
  Files          88       88              
  Lines        2110     2114       +4     
==========================================
+ Hits         1861     1865       +4     
  Misses        249      249              
Impacted Files Coverage Δ
src/types/wrappers/EnzymeConstrainedModel.jl 92.68% <ø> (ø)
src/reconstruction/enzyme_constrained.jl 100.00% <100.00%> (ø)
src/reconstruction/pipes/enzymes.jl 100.00% <100.00%> (ø)
...rc/reconstruction/simplified_enzyme_constrained.jl 100.00% <100.00%> (ø)
...types/wrappers/SimplifiedEnzymeConstrainedModel.jl 66.66% <100.00%> (ø)

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.

@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch 2 times, most recently from f8b03cf to e7d7798 Compare February 18, 2023 10:42
@stelmo
Copy link
Collaborator Author

stelmo commented Feb 18, 2023

/format

@github-actions
Copy link
Contributor

✔️ Auto-formatting triggered by this comment succeeded, commited as a3c4fc7

github-actions bot pushed a commit that referenced this pull request Feb 18, 2023
triggered by @stelmo on PR #759
@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch from e6c3d2b to f72d662 Compare February 18, 2023 11:04
@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch from f72d662 to 3edaa78 Compare February 18, 2023 11:06
@stelmo stelmo force-pushed the mo-homogenize-kwargs-enzyme-constrained branch from 09d9f45 to 68f46f3 Compare February 18, 2023 12:01
Copy link
Collaborator

@exaexa exaexa left a comment

Choose a reason for hiding this comment

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

ok

@stelmo stelmo merged commit 05f3881 into next Feb 18, 2023
@stelmo stelmo deleted the mo-homogenize-kwargs-enzyme-constrained branch February 18, 2023 15:23
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