Refactor/tests #1191

akaviaLab · 2022-03-23T15:01:59Z

fix Move the test suite out of the source tree #974
description of feature/fix
Move the test suite from src/cobra/test to src/tests. Also removed init.py from the test suite.
add an entry to the next release
Moved the test suite out of the source tree

Looks like this broke the test suite. Odd. Tox worked on my computer.

codecov-commenter · 2022-03-23T21:13:36Z

Codecov Report

Merging #1191 (969f4ea) into devel (b628572) will decrease coverage by 0.74%.
The diff coverage is 85.71%.

❗ Current head 969f4ea differs from pull request most recent head e13e8df. Consider uploading reports for the commit e13e8df to get more accurate results

@@            Coverage Diff             @@
##            devel    #1191      +/-   ##
==========================================
- Coverage   84.40%   83.66%   -0.75%     
==========================================
  Files          65       66       +1     
  Lines        5497     5511      +14     
  Branches     1263     1265       +2     
==========================================
- Hits         4640     4611      -29     
- Misses        557      598      +41     
- Partials      300      302       +2

Impacted Files	Coverage Δ
src/cobra/core/dictlist.py	`89.82% <ø> (ø)`
src/cobra/core/metabolite.py	`65.95% <ø> (ø)`
src/cobra/core/model.py	`87.80% <ø> (ø)`
src/cobra/core/reaction.py	`86.89% <ø> (ø)`
src/cobra/flux_analysis/gapfilling.py	`92.70% <ø> (ø)`
src/cobra/flux_analysis/phenotype_phase_plane.py	`83.00% <ø> (ø)`
src/cobra/io/json.py	`68.75% <60.00%> (+1.00%)`	⬆️
src/cobra/io/yaml.py	`56.41% <75.00%> (+1.14%)`	⬆️
src/cobra/io/web/cobrapy_repository.py	`100.00% <100.00%> (ø)`
src/cobra/io/web/load.py	`84.78% <100.00%> (+1.06%)`	⬆️
... and 6 more

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 b628572...e13e8df. Read the comment docs.

synchon · 2022-03-24T12:24:25Z

I believe what @Midnighter meant in #974 was to move src/cobra/test/ to tests/ (or, test/). Personally, I would not do this until we have completed the codebase modernization and flake8 is running in CI/CD. As this move will migrate data files and the tests, this will also need to take care of cobra.test.create_test_model(). Maybe others can also add their opinion about the move.

akaviaLab · 2022-03-24T17:57:45Z

Okay.
I can move it from src/tests to tests.

However, that will cause a bit of a problem since importing/using this in cobra without the source directory (as it is installed by a package) would be difficult. Two options for this are

Put the function create_model in src/scripts so it is installed locally
Put the function create_model in cobra.util directory (for example) and have it imported into tests. Tests can import from cobra, but removing init.py means that cobra has a hard time importing from tests.

I can use Docker to check it to make sure it is present and workable, including data, but I'd like a decision from the other devs on location please, if that's possible.

Midnighter · 2022-03-28T19:35:36Z

Indeed, my goal there was to have a /tests directory next to the /src/cobra directory. And I think it's actually a feature that the test suite is not installed with the source package.

The load_model function should be the replacement for create_test_model in documentation and examples everywhere. I think changing examples is the real major piece of work and the reason I didn't proceed on this so far. Within the test suite itself, we should have a few pytest fixtures that provide models.

akaviaLab · 2022-03-29T01:06:52Z

I think I see.
Would you like the cobra package to have some example data, so people can load_model even if there is no internet? Currently, create_test_model() has the advantage that it can work when no internet is available.

Midnighter · 2022-03-29T07:53:08Z

Would you like the cobra package to have some example data, so people can load_model even if there is no internet? Currently, create_test_model() has the advantage that it can work when no internet is available.

It's a valid concern. I'm not against packaging some models, also so that users have the exact models that examples were run with. Please note that in normal use load_model will cache the download locally so after first load, a connection will not be necessary. And you do need a connection to install cobrapy, of course.

akaviaLab · 2022-03-29T13:10:53Z

Is there any way to setup the package so that some example models are already loaded in the cache? I don't think so, and it might not be a good idea.
However, if it is possible and a reasonable idea, one or two models can start cached.

Alternatively, a possible (but not great) idea is to add some edge cases to load_model, so if it gets "textbook", which isn't in any of the repositories, it will load it locally from cobra data.

I'm just trying to think of users that are very new to cobrapy, and want to load some models to see behavior.

Midnighter · 2022-03-29T13:43:56Z

Is there any way to setup the package so that some example models are already loaded in the cache? I don't think so, and it might not be a good idea.
However, if it is possible and a reasonable idea, one or two models can start cached.

It can be done as a post-installation step but it's a bad idea, I think, because it assumes that the cache directory can be written to (this is not always the case).

Adding a utility function and distributing the models with the package is the best option, IMO. The only question is then if they should be in SBML or pickle format.

cdiener · 2022-03-29T16:25:20Z

I think we could get rid of the pickle ones because those are a pain to maintain anyways. I think having some manually validated JSON and YAML ones would be good though to reproduce bugs etc. I agree with using load_model just have to make sure it's a new release (not backwards compatible). The test models could act like a new provider "local" or "cobrapy" similar to the bio models one.

akaviaLab · 2022-03-29T16:46:26Z

Are the mini models validated?
I was thinking about getting update_pickles.py to copy some files to src/data. Set update_pickles.py to run before release.
Then the source version is still in tests/data, and that gets modified.

Or I could copy the mini models manually to src/data, and those would be examples.

So, load_models should have a repository like BiggModels which will be cobrapy, and that will use import_resources to import files. I think import_resources makes sure that however the users install the package, the data files are available.

akaviaLab · 2022-03-30T15:16:30Z

I fixed all the usages of create_test_model in the code examples.

I'd be happy to fix the python notebooks, but can you please tell me what needs to happen?
Do I just need to replace create_test_model with the appropriate call (simple text replacing like the examples)
OR do I need to manually run the Jupyter notebooks

Thank you

Midnighter · 2022-04-26T08:54:09Z

Dear @akaviaLab,

I would love to get this one and your other works merged. I tried to send you an email in order to set up a call to discuss this and the compartments PR but it came back with access denied. Can you please write to me or head over to https://gitter.im/opencobra/cobrapy where we can talk a bit more disconnected from code.

synchon · 2022-04-26T10:09:59Z

I think changing examples is the real major piece of work and the reason I didn't proceed on this so far.

@Midnighter Can we maybe take this opportunity to have our how-to Jupyter notebooks hosted on Binder ? I believe this will let new users to play around easily with cobrapy without hitting Python and related hurdles (we have seen quite a few 😅).

I think having some manually validated JSON and YAML ones would be good though to reproduce bugs etc.

@cdiener Would that be a good reason to have a separate repository for them or do we continue with as it is now?

Midnighter · 2022-04-26T10:14:00Z

@Midnighter Can we maybe take this opportunity to have our how-to Jupyter notebooks hosted on Binder ? I believe this will let new users to play around easily with cobrapy without hitting Python and related hurdles (we have seen quite a few sweat_smile).

Yes, can't remember where but Binder was mentioned and approved before by everyone involved. Just™️ need to do it 😉

synchon · 2022-04-26T10:17:53Z

Yes, can't remember where but Binder was mentioned and approved before by everyone involved. Just™️ need to do it 😉

Okay then I can take a stab at it in a few days 😁

akaviaLab · 2022-04-26T13:04:48Z

@Midnighter Posted on gitter
@synchon can you wait a bit before starting to work on binder? I"m working on this refactoring and discussing it with @Midnighter. Thank you

akaviaLab · 2022-04-26T14:50:54Z

All the notebook files done, except for faq.ipynb, which had functions I couldn't figure out.

synchon · 2022-04-26T15:07:52Z

@synchon can you wait a bit before starting to work on binder? I"m working on this refactoring and discussing it with @Midnighter. Thank you

Yes of course. I would have waited anyway to get this PR merged. 😄

akaviaLab · 2022-04-26T15:10:39Z

@synchon Great!
If you can review faq.ipynb, I'd appreciate it.

synchon · 2022-04-28T09:11:24Z

documentation_builder/faq.ipynb

-    "import cobra.test\n",
-    "model = cobra.test.create_test_model()\n",
+    "import cobra.io\n",
+    "model = cobra.io.load_model(\"iYS1720\")\n",
    "\n",
    "for metabolite in model.metabolites:\n",
    "    metabolite.id = \"test_\" + metabolite.id\n",


Would be nice to use f-string here to keep up with modern Python.

synchon · 2022-04-28T09:29:02Z

@akaviaLab faq.ipynb looks okay after your changes.

One comment about the section How do I generate an LP file from a COBRA model?: The solvers are now optlang-based and internal cobrapy solvers don't exist anymore, so better to remove the unnecessary sub-section. While you are at it, maybe also rename COBRA to cobrapy just to be consistent.

Also, this line With optlang solvers, the LP formulation of a model is obtained by it's string representation. All solvers behave the same way. has a small typo: ... obtained by its string .... Would be nice if you can fix that as well.

Any other enhancements, can be tackled later in a different PR later.

akaviaLab · 2022-04-28T16:09:10Z

Fixed comments. Set it up to be based on the most recent version of #1208 so it can be merged afterwards. If anything else is merged before this is done. I will rebase it.

synchon

Looks good. Thanks for the effort!

documentation_builder/deletions.ipynb

documentation_builder/constraints_objectives.ipynb

documentation_builder/faq.ipynb

src/cobra/manipulation/delete.py

tests/test_io/test_pickle.py

benchmarks/cobra-cameo-merge-benchmarks-before-merge.ipynb

benchmarks/cobra-cameo-merge-benchmarks.ipynb

akaviaLab

Fixed your comments. When the review is done, I will merge the changes to the mat file and resolve conflict - not doing it now, since it will break conversations.

benchmarks/cobra-cameo-merge-benchmarks.ipynb

benchmarks/cobra-cameo-merge-benchmarks-before-merge.ipynb

src/cobra/manipulation/delete.py

tests/test_io/test_json.py

Midnighter · 2022-04-29T10:21:43Z

@Midnighter - would you like me to squash the commits of the notebooks? I had one commit for each notebook code and one commit for each notebook output. I can squash them to one commit per notebook if that will help.

No, that actually sounds good to me.

Midnighter · 2022-04-29T10:25:07Z

I see now that you moved things to /src/tests rather than /tests. Would it have a huge impact on the other commits if you changed that now?

akaviaLab · 2022-04-29T10:35:53Z

Check a few commits down First I moved to stc/tests and then I moved to /tests

…

On Fri., Apr. 29, 2022, 6:25 a.m. Moritz E. Beber, ***@***.***> wrote: I see now that you moved things to /src/tests rather than /tests. Would it have a huge impact on the other commits if you changed that now? — Reply to this email directly, view it on GitHub <#1191 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQYYZXZSTSCVSC3YRCKP7DVHO2I5ANCNFSM5ROKAPJQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Midnighter

Phew, thank you so much for tackling this 🙂 I have a few more suggestions but overall it looks very good.

One bigger point is the exact models that we want to distribute with COBRApy. Seems like only the SBML files are needed there but maybe I overlooked something.

We should also look at including jupytext because those notebooks diffs are just painful. That's not for this PR, though.

Midnighter · 2022-04-29T11:09:55Z

setup.cfg

@@ -79,8 +79,8 @@ where = src

 [options.package_data]
 cobra =
-	io/*.json
-	test/data/*
+    data/*


I would like to reduce the models that we distribute with the package. Most of those are only really useful for testing and should be part of the repository but not the distributed Python package, I think.

Essentially, keep all SBML models for the examples and the others should be moved into a tests/data directory or so.

Hmm. I kept yaml, json, pickle and mat in the mini model format, just so we can show what they're supposed to look like.
I can definitely delete iJO1366.pickle.
As for the mini models - I'll agree to what you want.

Fair enough, they hardly hurt. Then just remove the pickle file, please.

src/cobra/io/web/cobrapy_repository.py

release-notes/next-release.md

src/cobra/io/web/load.py

tests/conftest.py

tests/test_io/test_pickle.py

tests/test_io/test_yaml.py

Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

using tmp_path instead of py.test (need to remove os.join in later PR)

Midnighter · 2022-04-29T14:29:59Z

Looks good to me now. As soon as @synchon is happy, we can merge it. Cheers @akaviaLab

akaviaLab · 2022-04-29T14:33:22Z

Might need to remove test_all I was waiting in that

…

On Fri., Apr. 29, 2022, 10:30 a.m. Moritz E. Beber, < ***@***.***> wrote: Looks good to me now. As soon as @synchon <https://github.com/synchon> is happy, we can merge it. Cheers @akaviaLab <https://github.com/akaviaLab> — Reply to this email directly, view it on GitHub <#1191 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACQYYZTT5UFVEO4PGEAPYWTVHPW7DANCNFSM5ROKAPJQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

synchon

Perfect! Let's merge this. 🎉

synchon · 2022-04-29T14:34:19Z

@Midnighter can we make a release after merging this PR?

Midnighter · 2022-04-29T17:09:23Z

@Midnighter can we make a release after merging this PR?

Sure

Might need to remove test_all

Yep, please do.

akaviaLab closed this Mar 30, 2022

akaviaLab reopened this Mar 30, 2022

akaviaLab force-pushed the refactor/tests branch from 9350013 to 0acc4c9 Compare April 20, 2022 16:37

akaviaLab force-pushed the refactor/tests branch from f32f611 to fd359c1 Compare April 26, 2022 14:50

synchon reviewed Apr 28, 2022

View reviewed changes

akaviaLab force-pushed the refactor/tests branch from fd359c1 to bc0781b Compare April 28, 2022 16:08

synchon requested changes Apr 28, 2022

View reviewed changes

akaviaLab commented Apr 28, 2022

View reviewed changes

benchmarks/cobra-cameo-merge-benchmarks.ipynb Show resolved Hide resolved

benchmarks/cobra-cameo-merge-benchmarks-before-merge.ipynb Show resolved Hide resolved

src/cobra/manipulation/delete.py Outdated Show resolved Hide resolved

synchon reviewed Apr 28, 2022

View reviewed changes

tests/test_io/test_json.py Outdated Show resolved Hide resolved

uri.akavia added 14 commits April 29, 2022 05:52

pymatbridge.ipynb code

fc8f5c5

sampling.ipynb code

be13970

sampling.ipynb output

6e998d8

solvers.ipynb code

b98e281

simulating.ipynb code

f8e9b2d

simulating.ipynb output

07092c9

fixed faq.ipynb and removed irrelevant section

f7e768a

fixed according to comments

64b6bec

changes in io from tests/data to src/cobra/data

77c7b21

io.ipynb should work now

5ee579f

fixed py.path instead of py.test

e4268ab

delet pymatbridge.ipynb

f72b78f

fixed more merge problems

f931815

fixed changes in compare_models

b4bc3a6

akaviaLab force-pushed the refactor/tests branch from 9c47562 to b4bc3a6 Compare April 29, 2022 09:57

Midnighter requested changes Apr 29, 2022

View reviewed changes

akaviaLab and others added 3 commits April 29, 2022 09:24

Apply suggestions from code review - mostly path

8a8b2cd

Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

deleted iOJ1366.pickle

868bfe1

using tmp_path instead of py.test (need to remove os.join in later PR)

isort black and flake8

a4bc963

Midnighter approved these changes Apr 29, 2022

View reviewed changes

synchon approved these changes Apr 29, 2022

View reviewed changes

removed test_all and pytest_addoption which seem irrelevant

e13e8df

Midnighter merged commit 91dfcfc into opencobra:devel Apr 29, 2022

Refactor/tests #1191

Refactor/tests #1191

Conversation

akaviaLab commented Mar 23, 2022 • edited Loading

codecov-commenter commented Mar 23, 2022 • edited Loading

Codecov Report

synchon commented Mar 24, 2022

akaviaLab commented Mar 24, 2022

Midnighter commented Mar 28, 2022

akaviaLab commented Mar 29, 2022

Midnighter commented Mar 29, 2022

akaviaLab commented Mar 29, 2022

Midnighter commented Mar 29, 2022

cdiener commented Mar 29, 2022

akaviaLab commented Mar 29, 2022

akaviaLab commented Mar 30, 2022

Midnighter commented Apr 26, 2022

synchon commented Apr 26, 2022

Midnighter commented Apr 26, 2022

synchon commented Apr 26, 2022

akaviaLab commented Apr 26, 2022

akaviaLab commented Apr 26, 2022

synchon commented Apr 26, 2022

akaviaLab commented Apr 26, 2022

synchon Apr 28, 2022

Choose a reason for hiding this comment

synchon commented Apr 28, 2022

akaviaLab commented Apr 28, 2022

synchon left a comment

Choose a reason for hiding this comment

akaviaLab left a comment

Choose a reason for hiding this comment

Midnighter commented Apr 29, 2022

Midnighter commented Apr 29, 2022

akaviaLab commented Apr 29, 2022 via email

Midnighter left a comment

Choose a reason for hiding this comment

Midnighter Apr 29, 2022

Choose a reason for hiding this comment

Midnighter Apr 29, 2022

Choose a reason for hiding this comment

akaviaLab Apr 29, 2022

Choose a reason for hiding this comment

Midnighter Apr 29, 2022

Choose a reason for hiding this comment

Midnighter commented Apr 29, 2022

akaviaLab commented Apr 29, 2022 via email

synchon left a comment

Choose a reason for hiding this comment

synchon commented Apr 29, 2022

Midnighter commented Apr 29, 2022

akaviaLab commented Mar 23, 2022 •

edited

Loading

codecov-commenter commented Mar 23, 2022 •

edited

Loading