Reaction balancing v2 #43
Conversation
There was a problem hiding this comment.
My overall comments:
- It would be helpful to have in the body of the PR (you should be able to edit it?) a description of the overall idea of the balancing algorithm. I think I roughly get that the right hand side and the left hand side of the reaction are balanced separately and then combined? But I'm not really sure.
- Also, why is
balancerxnrecursive? - In terms of style, I've made some notes in different places but generally python has a specific naming convention we should follow: https://google.github.io/styleguide/pyguide.html#316-naming
- Finally, can you add some tests to a file named
test/test_balancing.pyfollowing the convention from the other test files of creating functions starting withtest_? That way pytest will automatically pick up on the tests and run them in Github actions whenever new changes are pushed.
Also, it might be best to close this PR and create a new one with you as the author.
| agterrordict={} | ||
|
|
||
| # Generate reactant and product dictionaries (raise error if SMILES issue or missing reactants/products) | ||
| if getattr(reaction,"reactant_compounds"): |
There was a problem hiding this comment.
Why do you need to check if reactant_compounds if it is part of the Reaction API?
| IP (Optional[Dict], optional): Input parameters. Defaults to IP. | ||
| **kwargs: Additional input parameters. IP will be updated with these. |
There was a problem hiding this comment.
I would prefer if most of the arguments were in the function definition for two reasons:
- It enables autocomplete/type ahead in code editors to help people discover options without having to always look in the documentation
- It improves the automatic documentation (once we get there)
Also, please add type hints to the arguments. I'll get this all written up in a style guide at some point.
| def balance_reaction(reaction:Reaction, IP:Optional[Dict]=IP, rctstorgts=True, **kwargs): | ||
| """_summary_ | ||
|
|
||
| Args: |
There was a problem hiding this comment.
Let's follow the scipy style guide: https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard
| Dict: Compound dictionary with ID as key and another dictionary with the | ||
| following keys: 'atomdict' (Output of atomtypes, dictionary with atom | ||
| elements and count), 'charge' (Overall charge of mol),'smiles' | ||
| (smiles of mol),'formula' (chemical formula),'count' (instance number, always 1) | ||
| _type_: _description_ |
There was a problem hiding this comment.
I would prefer if this function returned an updated Reaction object.
|
|
||
|
|
||
| def balancerxn( | ||
| Rdata, |
There was a problem hiding this comment.
these should be snake case -> follow this style guide on naming: https://google.github.io/styleguide/pyguide.html#316-naming
No description provided.