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

adds div and simplifications for div, mod and rem #418

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

rmsrosa
Copy link

@rmsrosa rmsrosa commented Jan 13, 2022

I also added a bunch of tests, which highlight the simplifications added.

There are still some simplifications that can be included. I will work on those, but I think/hope this is a good start.

I haven't added anything to the documentation either. I can work on that later once I figure out how it is deployed.

Let me know if there is anything I should do differently. In particular regarding the way I added the rules to default_simplifier. I wanted to do minimal changes to that, but I suspect separating div/rem/mod from trig/exp simplifications might be best since they will certainly be less used.

@rmsrosa
Copy link
Author

rmsrosa commented Jan 14, 2022

Ok, I made my last commit of the series. I added the missing simplifications and adjusted testing. It is good to go.

src/simplify_rules.jl Outdated Show resolved Hide resolved
src/simplify_rules.jl Outdated Show resolved Hide resolved
@rmsrosa rmsrosa requested a review from shashi January 18, 2022 11:01
@rmsrosa rmsrosa requested a review from shashi February 1, 2022 14:34
@rmsrosa
Copy link
Author

rmsrosa commented Feb 17, 2022

Hmm, is there anything missing from my part? It still shows a change request but I believe I addressed that, already. I thought this was good to go. Let me know if I am missing something.

@YingboMa
Copy link
Member

@shashi could you take a look?

@codecov-commenter
Copy link

Codecov Report

Merging #418 (d994a48) into master (9bbd946) will decrease coverage by 0.60%.
The diff coverage is 48.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
- Coverage   83.99%   83.39%   -0.61%     
==========================================
  Files          13       13              
  Lines        1387     1409      +22     
==========================================
+ Hits         1165     1175      +10     
- Misses        222      234      +12     
Impacted Files Coverage Δ
src/methods.jl 83.63% <ø> (ø)
src/simplify_rules.jl 40.00% <22.22%> (-51.67%) ⬇️
src/utils.jl 65.00% <100.00%> (+7.25%) ⬆️
src/polyform.jl 93.72% <0.00%> (-0.42%) ⬇️
src/ordering.jl 89.65% <0.00%> (+1.14%) ⬆️

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 9bbd946...d994a48. Read the comment docs.

@rmsrosa
Copy link
Author

rmsrosa commented Feb 18, 2022

Ok, I can take a look at the codecov stuff. Maybe some rules are redundant or tests were not exhaustive.

But I don't know about the failed IntegrationTest/Symbolics.jl. Was that introduced by these changes or existed already?

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

4 participants