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

User defined constraints #1228

Closed
wants to merge 9 commits into from

Conversation

akaviaLab
Copy link
Contributor

  • fix User defined constraints #996
    This is an updated version of User defined constraints #996 that contains only the changes needed for user defined constraints. Some corrections like iteritems to items(), etc.
    I think the original PR doesn't use optlang correctly. There is a lot of ast parsing, which I'm not sure is necessary.
    I don't understand optlang that well, so if it needs to be rewritten, I would need a starting point. I think that it can just be replaced with optlang symbolic parsing and leave the ast to optlang (so just remove that code).

@matthiaskoenig - I think that as this point libsbml supports fbc with user defined constraints. Is that correct?

  • tests added/passed
    This PR adds some tests for adding user constraints.
  • add an entry to the next release

uri.akavia added 3 commits May 30, 2022 09:37
@codecov-commenter
Copy link

codecov-commenter commented May 30, 2022

Codecov Report

Merging #1228 (d5fea10) into devel (7d97819) will decrease coverage by 1.50%.
The diff coverage is 58.13%.

❗ Current head d5fea10 differs from pull request most recent head 1a6bca6. Consider uploading reports for the commit 1a6bca6 to get more accurate results

@@            Coverage Diff             @@
##            devel    #1228      +/-   ##
==========================================
- Coverage   84.44%   82.94%   -1.51%     
==========================================
  Files          66       67       +1     
  Lines        5491     5822     +331     
  Branches     1264     1363      +99     
==========================================
+ Hits         4637     4829     +192     
- Misses        550      648      +98     
- Partials      304      345      +41     
Impacted Files Coverage Δ
src/cobra/core/model.py 82.26% <38.80%> (-5.55%) ⬇️
src/cobra/core/udconstraints.py 58.40% <58.40%> (ø)
src/cobra/io/dict.py 89.67% <89.47%> (-0.16%) ⬇️
src/cobra/core/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d97819...1a6bca6. Read the comment docs.

@cdiener
Copy link
Member

cdiener commented Jun 6, 2022

Wasn't there a different PR that refactored this already? I think one of the issues was the addition of a new constraint accessor even though custom constraints are already a part of COBRAPY via optlang. The issue is that SBML only supports a subset of those constraints since it does not put the problem in standard form (all v>0, separate forward and reverse fluxes), however it is fairly straight forward to check if an existing constraint can be expressed in FBC3 and that is probably the way to go (also see #988).

@akaviaLab
Copy link
Contributor Author

akaviaLab commented Jun 6, 2022 via email

@akaviaLab
Copy link
Contributor Author

see #1240

@akaviaLab
Copy link
Contributor Author

Now it is pure optlang. JSON import and export work. SBML, Mat, YAML next.

Almost all tests work. Some tests give different results with the optlang and the pure accessor (last two asserts in test_user_defined_constraints_with_variable_documented). I can't figure out what is wrong, and would appreciate help.

@akaviaLab akaviaLab closed this Jul 5, 2022
@matthiaskoenig
Copy link
Contributor

Sorry for the slow reply. Yes, libsbml implements all of the UserDefinedConstraints. This was part of the original pull request.

@akaviaLab
Copy link
Contributor Author

akaviaLab commented Jul 13, 2022 via email

@matthiaskoenig
Copy link
Contributor

You are right. There was no code in the original submission. I will implement the SBML serialization but it seems some issues still have to be solved. I just checked and the fbc version 3 code is in the current python-libsbml packages.
But unfortunately nobody tried this so far. So there are still bugs.

I am testing this currently outside of cobrapy for another project and as soon as the issues are resolved I will take care of the SBML serialization of the UserDefinedConstraints and the KeyValuePairs. See sbmlteam/libsbml#241 for an example problem I just encountered. I am implementing full serialization of the new features in sbmlutils and as soon as I am sure things are working I will implement the serialization here.

@akaviaLab
Copy link
Contributor Author

akaviaLab commented Jul 13, 2022 via email

@matthiaskoenig
Copy link
Contributor

matthiaskoenig commented Jul 13, 2022 via email

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.

4 participants