Check annotation to be of type dict while reading it from the json

[Hemant27031999]

What does it fixes:

The current version of code throws an error at the time of writing model in SBML format if annotations are of type list of lists. The JSON schema wants annotations to be of dictionary type, but the validation check of JSON is not performed using the schema because it will make the parsing slow. So I have updated cobra/io/object.py class by adding setter for annotation which checks if the annotation is of type dictionary or not at the time reading model from the JSON and throws a TypeError if it is not.

Files changed/added:

• Updated cobra/io/object.py.
• Added cobra/test/test_io/test_annotation_format.py test which tests the JSON format from the files cobra/test/data/valid_annotation_format.json and cobra/test/data/invalid_annotation_format.json of the annotation. If the annotation is in the right format, parsing is done and SBML of the model is written in a file called cobra/test/data/valid_annotation_output.xml.

[Hemant27031999]

@cdiener , I have implemented the needs for annotations format that were discussed on issue:736. I have also added the test cases to test the parsing. However, a few other tests are now producing error. One of them is: cobra/test/test_io/test_sbml.py. It has a test_read_2 test which compares models from two files. For eg: it compares models mini.pickle and mini_fbc2.xml using the function extra_comparisons() of same file. However, while comparing data from two files, the following error occurs :

@classmethod
def extra_comparisons(cls, name, model1, model2):
    assert model1.compartments == model2.compartments

    # FIXME: problems of duplicate annotations in test data
    #  ('cas': ['56-65-5', '56-65-5'])
    # assert dict(model1.metabolites[4].annotation) == dict(
    #    model2.metabolites[4].annotation)
    d1 = model1.reactions[4].annotation
    d2 = model2.reactions[4].annotation

  assert list(d1.keys()) == list(d2.keys())

E AssertionError: assert [] == ['sbo', 'bigg.reaction']
E Right contains 2 more items, first extra item: 'sbo'

Here as you can see, the two annotation lists are different (one is empty and other is ['sbo', 'bigg.reaction']) from the two models and hence a false assert is produced which fails the test. I tried to look into the corresponding models (mini.pickle and mini_fbc2.xml). I parsed the pickle into json but there is no annotation list, and there is one in mini_fbc2.xml file. Can you help me a little to understand why it is happening?

[cdiener]

Sure, I'll have a look during the week.

[Midnighter]

I currently don't have a good overview of where we use direct assignment to the annotation attribute. Overall, I think it would be cleaner to add a method that hides the internal data structure. Something like

def add_annotation(self, namespace: str, identifier: str, biology_qualifier="is"):

but as I said I don't know how much code would be affected by this.

[cdiener]

Sorry for the delay. At least some of the test failures come from pickled test models with annotation == None. Don't know where exactly that arises but for now you could try to rebuild the pickles as described in https://github.com/opencobra/cobrapy/blob/devel/.github/CONTRIBUTING.rst#faqs .

[Hemant27031999]

Thanks Sir for looking into the matter.

The error occurring here is due to the changes I made in Object class for annotation. I haven't repickled the models after modifying the Object class, that's why it is giving the annotation list as an empty list. So I tried to repickle the models using the modified Object class using the update_pickles.py file, but I think this file is outdated, and it throws the following error:

• it is using 'write_sbml2' method of 'cobra.io.sbml3' module in line 13, but neither this module nor this method (after checking in cobra.io.sbml) is present anywhere.
• it is using 'iJO1366.xml' model in line 29 but this model is present in compressed form i.e 'iJO1366.xml.gz', so it also throws an error.
• in line 63, 'D__LACt2pp' reaction is used from 'ecoli_model' (this model is made using iJO1366.xml), but model iJO1366 doesn't have any such reaction, so it throw an error here also.

Am I missing something or is it really outdated?

[Midnighter]

Am I missing something or is it really outdated?

Yeah, most likely outdated. It only really gets updated when someone needs to run it, like you do now 😉

[Hemant27031999]

Ok. Let me guess, what we will have to change in it is somewhat like this :

• Regarding the first error where 'write_sbml2' method is used to write the model in 'mini_fbc1.xml', is it still required anywhere or should it be removed? I looked for its usage and found one in 'test_sbml.py' file in line 41 i.e the fourth test case

              IOTrial('fbc1', 'mini.pickle', 'mini_fbc1.xml',
              read_sbml_model, write_sbml_model, None),
  

But even here also, it is not in any actual use. In the first test i.e

        def test_validate(trial, data_directory):
              """ Test validation function. """
              if trial.validation_function is None:
              pytest.skip('not implemented')
              test_file = join(data_directory, trial.test_file)
              trial.validation_function(test_file)

it gets skipped due to no provided validation_function for it. And similarly for further test cases also, it is either marked as 'pytest.xfail('not supported')' or skipped. So it is not actually used anywhere.

• Regarding the second error i.e using 'iJO1366.xml', I think replacing it with 'iJO1366.xml.gz' will do the required work.
• Regarding the third error i.e using reaction 'D__LACt2pp' from model iJO1366, I have some doubts. On the BiGG Model database, I can't find this reaction under model iJO1366. So should it be removed here also?
  Thanks.

[codecov-io]

Codecov Report

Merging #930 into devel will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            devel     #930      +/-   ##
==========================================
+ Coverage   84.44%   84.63%   +0.18%     
==========================================
  Files          50       50              
  Lines        4353     4438      +85     
  Branches      996      998       +2     
==========================================
+ Hits         3676     3756      +80     
- Misses        433      441       +8     
+ Partials      244      241       -3

Impacted Files	Coverage Δ
cobra/core/object.py	100% <100%> (ø)	⬆️
cobra/core/dictlist.py	89.23% <0%> (-2.25%)	⬇️
cobra/medium/minimal_medium.py	87.87% <0%> (-1.82%)	⬇️
cobra/core/species.py	100% <0%> (ø)	⬆️
cobra/io/sbml.py	80.04% <0%> (+0.27%)	⬆️
cobra/core/gene.py	74.35% <0%> (+0.44%)	⬆️
cobra/core/group.py	96.87% <0%> (+0.44%)	⬆️
cobra/core/model.py	88.56% <0%> (+0.49%)	⬆️
cobra/core/reaction.py	88.71% <0%> (+0.87%)	⬆️
... and 3 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 35dbddf...d1270e2. Read the comment docs.

[Hemant27031999]

Sir, I have updated update_pickle.py file as per the changes discussed above. About the third error which I mentioned, I have removed the reaction 'D__LACt2pp' from model iJO1366 and have updated the pickle and other resource files using the updated Object class. Changes suggested by @Midnighter have also been done. There was one test case that was failing i.e test_validate inside test_sbml.py (line 277) file. It was asking for 23 errors when validation is performed over mini_fbc3.xml resource model, but there were no errors generated after I have modified the Object class.
Please review my changes and let me know if some changes are required.
Thanks.

[Hemant27031999]

@cdiener , @Midnighter do you suggest some changes or is it fine?

[Midnighter]

Hi @Hemant27031999, sorry for the long wait on this one. I have a few minor comments, otherwise this PR looks good to me.

[Hemant27031999]

@Midnighter, I have made the changes. Please review.

[Midnighter]

Thank you for your work!

[Midnighter]

Let me know @cdiener when you're happy, too. I can merge it then and make a new release.

[cdiener]

I'm on board if it looks good to you. Seems like CI is failing again due to unsorted imports.