-
Notifications
You must be signed in to change notification settings - Fork 218
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
Metadata fbc3 group #988
base: devel
Are you sure you want to change the base?
Metadata fbc3 group #988
Conversation
Codecov Report
@@ Coverage Diff @@
## devel #988 +/- ##
==========================================
- Coverage 84.45% 83.13% -1.32%
==========================================
Files 58 64 +6
Lines 5036 5888 +852
Branches 1092 1276 +184
==========================================
+ Hits 4253 4895 +642
- Misses 508 660 +152
- Partials 275 333 +58
Continue to review full report at Codecov.
|
@Hemant27031999 I went over most of the code and cleaned it up. Was much more work then expected but most of the metadata is now pretty clean and neat. I also updated to the latest develop code. Could you
|
@matthiaskoenig I have made the requested changes. All tests are passing now. |
e77f9ab
to
67d3af1
Compare
Hey people, metadata and UserDefinedConstraints are two separate functionalities. So I have removed UserDefinedConstraint from this branch, it now majorly includes metadata and some JSON serialization. I shall link another PR for Constraint class. |
src/cobra/io/dict.py
Outdated
cvterms = data["cvterms"] if "cvterms" in data else None | ||
history = data["history"] if "history" in data else None | ||
keyValueDict = data["history"] if "key_value_data" in data else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to define String
literals as constants? Using such literal expressions repeatedly belongs to a possible source of error (because of typos) that can be relatively easily eliminated.
src/cobra/io/dict.py
Outdated
if "sbo" in data: | ||
annotation["sbo"] = [data["sbo"]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here also: Please avoid using String
literals repeatedly.
@@ -180,7 +186,7 @@ def _update_optional(cobra_object, new_dict, optional_attribute_dict, | |||
def metabolite_to_dict(metabolite): | |||
new_met = OrderedDict() | |||
for key in _REQUIRED_METABOLITE_ATTRIBUTES: | |||
if key == 'id': | |||
if key == "id": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again... Define as a String
constant if possible.
cobra_members.append(cobra_obj) | ||
elif member["type"] == "Gene": | ||
cobra_obj = model.genes.get_by_id( | ||
F_REPLACE["F_GENE"](member["idRef"])) | ||
F_REPLACE["F_GENE"](member["idRef"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just notice that there are many String
literals all over the source code. I will not add further remarks about it. If possible, please go through the source code, identify String
literals and replace them with constant expressions (constant variables).
@@ -295,7 +295,7 @@ def constraint_from_expression(id=None, expression: 'str' = '', | |||
tree = ast.parse(source=expression, mode='eval') | |||
compute_nodes = UserDefinedConstraint.ComputeNumericNodes() | |||
tree = compute_nodes.visit(tree) | |||
print(ast.dump(tree)) | |||
print((ast.dump(tree))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are additional parentheses needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I added them or the code formatter. By the way, I will update it. This file is actually a part of other the PR now. It's no longer on this branch.
result = Num(n=node.left.n + node.right.n) | ||
return copy_location(result, node) | ||
elif isinstance(node.op, Sub): | ||
result = Num(n=node.left.n - node.right.n) | ||
return copy_location(result, node) | ||
elif isinstance(node.op, Mult): | ||
result = Num(n=node.left.n * node.right.n) | ||
return copy_location(result, node) | ||
elif isinstance(node.op, Div): | ||
result = Num(n=node.left.n / node.right.n) | ||
return copy_location(result, node) | ||
elif isinstance(node.op, Mod): | ||
result = Num(n=node.left.n % node.right.n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add blanks between variables and operators, e.g., change n=xyz
to n = xyz
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python code format tool "black" doesn't allow spaces between the variable and operator when passed as arguments.
src/cobra/core/formula.py
Outdated
"P": 30.973761, | ||
"S": 32.065000, | ||
"Cl": 35.453000, | ||
"Ar": 39.948000, | ||
"K": 39.098300, | ||
"Ca": 40.078000, | ||
"Sc": 44.955910, | ||
"Ti": 47.867000, | ||
"V": 50.941500, | ||
"Cr": 51.996100, | ||
"Mn": 54.938049, | ||
"Fe": 55.845000, | ||
"Co": 58.933200, | ||
"Ni": 58.693400, | ||
"Cu": 63.546000, | ||
"Zn": 65.409000, | ||
"Ga": 69.723000, | ||
"Ge": 72.640000, | ||
"As": 74.921600, | ||
"Se": 78.960000, | ||
"Br": 79.904000, | ||
"Kr": 83.798000, | ||
"Rb": 85.467800, | ||
"Sr": 87.620000, | ||
"Y": 88.905850, | ||
"Zr": 91.224000, | ||
"Nb": 92.906380, | ||
"Mo": 95.940000, | ||
"Tc": 98.000000, | ||
"Ru": 101.070000, | ||
"Rh": 102.905500, | ||
"Pd": 106.420000, | ||
"Ag": 107.868200, | ||
"Cd": 112.411000, | ||
"In": 114.818000, | ||
"Sn": 118.710000, | ||
"Sb": 121.760000, | ||
"Te": 127.600000, | ||
"I": 126.904470, | ||
"Xe": 131.293000, | ||
"Cs": 132.905450, | ||
"Ba": 137.327000, | ||
"La": 138.905500, | ||
"Ce": 140.116000, | ||
"Pr": 140.907650, | ||
"Nd": 144.240000, | ||
"Pm": 145.000000, | ||
"Sm": 150.360000, | ||
"Eu": 151.964000, | ||
"Gd": 157.250000, | ||
"Tb": 158.925340, | ||
"Dy": 162.500000, | ||
"Ho": 164.930320, | ||
"Er": 167.259000, | ||
"Tm": 168.934210, | ||
"Yb": 173.040000, | ||
"Lu": 174.967000, | ||
"Hf": 178.490000, | ||
"Ta": 180.947900, | ||
"W": 183.840000, | ||
"Re": 186.207000, | ||
"Os": 190.230000, | ||
"Ir": 192.217000, | ||
"Pt": 195.078000, | ||
"Au": 196.966550, | ||
"Hg": 200.590000, | ||
"Tl": 204.383300, | ||
"Pb": 207.200000, | ||
"Bi": 208.980380, | ||
"Po": 209.000000, | ||
"At": 210.000000, | ||
"Rn": 222.000000, | ||
"Fr": 223.000000, | ||
"Ra": 226.000000, | ||
"Ac": 227.000000, | ||
"Th": 232.038100, | ||
"Pa": 231.035880, | ||
"U": 238.028910, | ||
"Np": 237.000000, | ||
"Pu": 244.000000, | ||
"Am": 243.000000, | ||
"Cm": 247.000000, | ||
"Bk": 247.000000, | ||
"Cf": 251.000000, | ||
"Es": 252.000000, | ||
"Fm": 257.000000, | ||
"Md": 258.000000, | ||
"No": 259.000000, | ||
"Lr": 262.000000, | ||
"Rf": 261.000000, | ||
"Db": 262.000000, | ||
"Sg": 266.000000, | ||
"Bh": 264.000000, | ||
"Hs": 277.000000, | ||
"Mt": 268.000000, | ||
"Ds": 281.000000, | ||
"Rg": 272.000000, | ||
"Cn": 285.000000, | ||
"Uuq": 289.000000, | ||
"Uuh": 292.000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that elements naturally occur in different isotopes. Please indicate if these values are the average or the most likely molecular weight in form of a user-readable description. Better could be to use a dictionary whose values are lists of values for the different isotopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Members, please provide review. I haven't updated it. It's been reformatted by "black" itself by adding double quotes in place of single quotes.
cobra/core/annotation.py
Outdated
QUALIFIER_TYPES = ( | ||
"is", "hasPart", "isPartOf", "isVersionOf", "hasVersion", | ||
"isHomologTo", "isDescribedBy", "isEncodedBy", "encodes", | ||
"occursIn", "hasProperty", "isPropertyOf", "hasTaxon", | ||
"unknown", "bqm_is", "bqm_isDescribedBy", "bqm_isDerivedFrom", | ||
"bqm_isInstanceOf", "bqm_hasInstance", "bqm_unknown", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a documentation comment here where to find the most recent list of qualifiers to ease later updats.
Description of Features
cobra/core
directory. Every object derived from SBase can have meta information like annotation (CVTerms), notes, history attached to it.Notes
class is holding a simple string containing notes data (XHTML string) and a dictionary, synchronized with the notes string, storing key-value pair of the form<p> key : value </p>
present in the notes string. One can only modify this key-value pair data, he can't add new key-value pairs inside notes because notes are not a right place to store these key-value pairs.CVTerms
class for storing externally linked resources to each component derived from SBase. This class is maintaining the new format annotation as well as old format annotation simultaneously, and both are kept synchronized with each other. Changing one will modify the other accordingly. This new class for annotation can handle any type of annotation data (be it the case of nested annotation or alternative annotation). It can read the old format as well as the new format annotation data from JSON and other formats. At the time of writing back the model, the new data format is used because it contains the complete data organized in the same way as SBML.History
class used for storing the history, validating dates, etc, is now attached to each component derived from SBase.KeyValuePair
class for storing key-value pairs, defined by fbc-v3.The last three metadata objects (i.e CVTerms, History, KeyValuePair) are present inside a single attribute of SBase (Object) class and can be accessed via
object.annotation.cvterms
,object.annotation.history
andobject.annotation.key_value_data
attributes. Calling simply the annotation attribute (object.annotation
) will return the annotation data in old format (making it backward compatible).Group to JSON: The support of the group package is extended to JSON.
JSON schema v2: The version2 of JSON schema has been added which defines the new format annotation, history, key-value pair, notes, group package data, user-defined constraints data and basic SBML info.
Issues Fixed
sbml_info
storing basic information of SBML is written to JSON to store the basic SBML document information like packages, level, version, notes, annotation attached to the SBML component etc.Tests
Tests for all the newly implemented features are added to check the functionalities. A few old tests are also modified accordingly. Some tests which were initially marked 'xfail' are now working dew to modified formats.