Skip to content
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

ENH: Simplify convert_math_to_symbolic #5

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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