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

Fix float exponents when writing a TDB file (TC ERROR DatabaseUtils) #516

Merged
merged 9 commits into from
Jun 24, 2024

Conversation

rdamaral
Copy link
Contributor

@rdamaral rdamaral commented Mar 6, 2024

Hi, this is a small fix to '_print_Piecewise' function in the io.tdb module to improve compatibility with TC. I'm using this module to parse some of my tdbs and TC complains about float exponents (A constant integer must be used as the exponent in exponential TDB expressions). Thank you.

Edit on 03/07: I updated my previous fix to change only the exponents in '_stringify_expr' and added some extra rules where to break lines in 'reflow_text'. I did a couple of tests with different tdbs and TC is reading it with no errors now.

Checklist

  • [N/A] If any dependencies have changed, the changes are reflected in the
    • [N/A] setup.py (runtime requirements)
    • [N/A] pyproject.toml (build requirements)
    • [N/A] requirements-dev.txt (build and development requirements)

Copy link

codecov bot commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.46%. Comparing base (1c79c0c) to head (1dc5cc1).

Current head 1dc5cc1 differs from pull request most recent head 7bf0bd4

Please upload reports for the commit 7bf0bd4 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #516      +/-   ##
===========================================
- Coverage    90.47%   90.46%   -0.02%     
===========================================
  Files           50       50              
  Lines         7878     7868      -10     
===========================================
- Hits          7128     7118      -10     
  Misses         750      750              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@richardotis
Copy link
Collaborator

Only one test is failing, and I think it's safe for you to modify the pass condition for that test to match this new behavior, since it only differs in the amount of whitespace.

…addition/subtraction that is not part of an EXP or LN/LOG operation. Updated the test in test_database.py to reflect changes made in tdb.py.
@rdamaral
Copy link
Contributor Author

Hi Richard, I’ve updated ‘test_database.py’, and the test should pass now. I’ve also generalized my previous rules to allow line breaks before any number addition/subtraction that isn’t part of an EXP or LN/LOG operation. The previous rules already accounted for that, but were overlooking some permissible break points.

@richardotis
Copy link
Collaborator

Please pull and merge the latest develop into this branch to resolve the conflict with #522

I think it should be an easy merge, just adopting the new code and changing the ' * ' to '*' as you've already done.

@rdamaral
Copy link
Contributor Author

Hi @richardotis. This regex basically matches any addition and subtraction of any number, ie. [+-][0-9], since they are not preceded by 'E' or '('. This should prevent breaking lines inside EXP or LN/LOG or power expression: '6.14599E-07', 'T**(-3)', and 'LOG(-3)', hence splitting these could lead to errors in TC. In practice, this makes the functions being breakable at the linebreak_chars plus at [+-][0-9], since they are not preceded by 'E' or '('.

I'll add a comment on the code about this implementation and merge it to #522

pycalphad/io/tdb.py Outdated Show resolved Hide resolved
@richardotis
Copy link
Collaborator

richardotis commented Jun 24, 2024

The test failures are unrelated and will be fixed once #530 is merged.

Can you add a test for reflow_text to confirm the line-breaking behavior for floating-point exponents is correct now?

@rdamaral
Copy link
Contributor Author

Sure, I'll add it and let you know.

@rdamaral
Copy link
Contributor Author

Hi @richardotis, just added a new test test_reflow_text_for_line_breaks for testing the line breaks and updated test_tc_printer_exp for testing if floating-point exponents are being returned.

@richardotis richardotis merged commit a3c1005 into pycalphad:develop Jun 24, 2024
11 of 24 checks passed
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.

None yet

2 participants