Skip to content

Conversation

@bocklund
Copy link
Collaborator

Some changes to convert_math_to_symbolic following on to discussion in #4. I think the root cause is of the weirdness is that convert_math_to_symbolic is oddly polymorphic, where math_nodes can be strings or nodes. I tried to split it up a little bit so there's not a circular call graph where convert_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:

  1. Zero or more intervals, or
  2. A text expression (where empty text ('') 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. the Expr/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 on interval_node). Except for cases where S.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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant