Skip to content

Feature/sympy simplify#24

Open
alexhagen wants to merge 8 commits into
erwanp:masterfrom
alexhagen:feature/sympy_simplify
Open

Feature/sympy simplify#24
alexhagen wants to merge 8 commits into
erwanp:masterfrom
alexhagen:feature/sympy_simplify

Conversation

@alexhagen

Copy link
Copy Markdown
Contributor

Working simplification for Issue #22. Uses sympy to do the simplification.

Requested use case: py2tex('theta = w_k * a ** 4 / t ** 4 / E', simplify_fractions=True) now outputs $$\theta=\frac{a^{4} w_{k}}{E t^{4}}$$.

@erwanp

erwanp commented Feb 3, 2021

Copy link
Copy Markdown
Owner

There seems to be an index problem in the "sum ... until 20" test, which has been corrected to sum([... for i in range(20)]) although it is actually sum([... for i in range(21)]) in Python

@alexhagen

Copy link
Copy Markdown
Contributor Author

Oh interesting. I'll check into that.

@alexhagen

Copy link
Copy Markdown
Contributor Author

@erwanp I've fixed the index problem you pointed out, I think I must've forked from an old version. Let me know if anything else needs fixed.

@erwanp

erwanp commented Feb 8, 2021

Copy link
Copy Markdown
Owner

I think you can fix the last conflict from the online interface. Else I'll do it!

@codecov

codecov Bot commented Feb 8, 2021

Copy link
Copy Markdown

Codecov Report

Merging #24 (ae93b58) into master (b18e0f6) will decrease coverage by 1.67%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #24      +/-   ##
==========================================
- Coverage   81.09%   79.41%   -1.68%     
==========================================
  Files          13       13              
  Lines         603      617      +14     
==========================================
+ Hits          489      490       +1     
- Misses        114      127      +13     
Impacted Files Coverage Δ
setup.py 0.00% <ø> (ø)
pytexit/core/core.py 87.17% <4.54%> (-3.41%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b18e0f6...ae93b58. Read the comment docs.

@erwanp

erwanp commented Feb 8, 2021

Copy link
Copy Markdown
Owner

@alexhagen

Test crash with :

I think we should maintain compatibility with 2.7 for some time, but this feature is nice. Can you move the import within the function maybe ?

@alexhagen

Copy link
Copy Markdown
Contributor Author

For sure. Should I break this off from your simplify_fractions parameter, we could have another parameter use_sympy?

@erwanp

erwanp commented Feb 9, 2021

Copy link
Copy Markdown
Owner

I'd say keep it within simplify_fractions. #22 OP @turner-eng expected to find this behavior. Less parameters also make the code easier to understand. We could escape the simplify_fraction test with sys.version_info[0]

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.

3 participants