ENH: Simplify convert_math_to_symbolic #5
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some changes to
convert_math_to_symbolic
following on to discussion in #4. I think the root cause is of the weirdness is thatconvert_math_to_symbolic
is oddly polymorphic, wheremath_nodes
can be strings or nodes. I tried to split it up a little bit so there's not a circular call graph whereconvert_math_to_symbolic
(with interval nodes) ->convert_intervals_to_piecewise
->convert_math_to_symbolic
(with a string node). I'm not sure if it makes sense without breaking the intent of how to handle intervals (and possible nesting of intervals, although I think that is currently broken).If my understanding is correct, I think it should be that any incoming node to
convert_math_to_symbolic
will have:''
) implies zero) that is additive to (1).Is there a practical difference between how we treat the Interval text (
''.join(interval_node.itertext()).replace('\n', '').replace(' ', '').strip()
) vs. theExpr
/Parameter
text (''.join(node.xpath('./text()')).replace('\n', '').replace(' ', '').strip()
)? Maybe for nested intervals?The current test suite is admittedly small, but I verified that the text in both cases is the same across the test suite (if you use
xpath
oninterval_node
). Except for cases whereS.Zero
!=Float(S.Zero)
, the current test suite still passes after this change. I have a corresponding pycalphad patch lined up to coerce all numeric values in Gibbs energy expressions to symengine Floats in the DAT.