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

Adding metComps and utilities for Annotations #1396

Merged
merged 33 commits into from
Apr 2, 2019

Conversation

tpfau
Copy link
Contributor

@tpfau tpfau commented Nov 29, 2018

This PR adds further utility functions for annotations and updates the existing functions (i.e. add/getMIRIAMAnnotation) to be more flexible with respect to outputs (potentially allowing this to be used during IO).
It also introduces the metComps field and adjusts several things required for this change.
I'm not sure, whether I caught all fields which rely on the metabolite names, but I actually haven't found many that do. And those that did were adjusted.
We will still need to "handle" compartment IDs in id names, if we want to find the "basic" metabolite for them (or rely on external ids for that purpose) but this PR should address at least some of it.

I hereby confirm that I have:

  • Tested my code on my own machine
  • Followed the guidelines in the Contributing Guide
  • Selected integrationRAVEN as a target branch (top left drop-down menu)

(Note: You may replace [ ] with [X] to check the box)

Copy link
Member

@rmtfleming rmtfleming left a comment

Choose a reason for hiding this comment

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

extractCompartmentsFromMets could do with some usage examples

@rmtfleming
Copy link
Member

@ithiele this may affect Harvey computations.
@laurentheirendt It needs checked before we can accept this PR.

@codecov-io
Copy link

codecov-io commented Nov 30, 2018

Codecov Report

Merging #1396 into integrationRAVEN will decrease coverage by 0.49%.
The diff coverage is 70.36%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           integrationRAVEN    #1396     +/-   ##
===================================================
- Coverage             44.21%   43.72%   -0.5%     
===================================================
  Files                   904      901      -3     
  Lines                 64715    64048    -667     
===================================================
- Hits                  28613    28004    -609     
+ Misses                36102    36044     -58
Impacted Files Coverage Δ
.../analysis/thermo/vonBertalanffy/setupThermoModel.m 0% <ø> (ø) ⬆️
...reconstruction/refinement/addMultipleMetabolites.m 90.62% <ø> (ø) ⬆️
...thermo/vonBertalanffy/setupComponentContribution.m 0% <ø> (ø) ⬆️
src/reconstruction/refinement/extractSubNetwork.m 58.82% <ø> (ø) ⬆️
src/base/io/utilities/createEmptyFields.m 94.28% <ø> (+5.71%) ⬆️
...construction/refinement/mergeModelFieldPositions.m 86.53% <ø> (ø) ⬆️
src/analysis/thermo/vonBertalanffy/estimateDrGt0.m 0% <0%> (ø) ⬆️
...ion/modelGeneration/massBalance/pHbalanceProtons.m 0% <0%> (ø) ⬆️
src/analysis/exploration/findTrspRxnFromMet.m 0% <0%> (ø) ⬆️
src/reconstruction/refinement/setAnnotations.m 0% <0%> (ø)
... and 111 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 d24e0e2...889e8f6. Read the comment docs.

@laurentheirendt
Copy link
Contributor

@tpfau, is there any particular test that ensures backward compatibility?

@tpfau
Copy link
Contributor Author

tpfau commented Jan 11, 2019

It passes all existing tests with only minimal modifications which are mainly due to the new fields being added and equality comparisons between existing models and manually manipulated models, as well as ensuring that the new fields are covered by the tests.
Not sure what kind of other backward compatability tests could be used for otherwise untested functionality.

@laurentheirendt
Copy link
Contributor

@tpfau, may you do a rebase here? Thanks 👍

@laurentheirendt
Copy link
Contributor

@tpfau, may you do a rebase here? Thanks.

@laurentheirendt
Copy link
Contributor

This can actually be safely merged to integrationRAVEN. Further tests will be needed once that branch will be merged to develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants