avaucher
commented
2 years ago My personal choice would be not to do that. A few points of explanation:
- I see the conversion to string, the canonicalization, and the removal of atom mapping as three distinct things. To me the
to_stringfunction should really be about that: the conversion to a string. [PS1] - This would raise the question of adding it to all the reaction-to-string conversions, i.e. also the one with extended SMILES
- I try to avoid confusion by keeping the number of arguments small
- Linked to the last point: this may lead to other potential arguments such as sorting, removal of duplicates, etc.
Having said that, I am more open to having an external function doing multiple of those if needed. Something along the lines of cleanup_reaction(reaction_equation)? Here I would prefer to work on the reaction equation as on the str, as it is compatible with extended SMILES for instance. Also, I tend to prefer free non-member functions when possible.
What are your thoughts on this?
[PS1]: following up on the arguments with free functions and other kinds of strings, I was already tempted a few times to remove to_string from the ReactionEquation class 😉
Related to https://github.com/rxn4chemistry/rxn-chemutils/issues/8
proposal to add canonicalisation and removal of atom mapping as arguments to the to_string method of ReactionEquation reaction.to_string(canonicalise=True, remove_atom_mapping=True)