Open rmsrosa opened 2 years ago
Ok, I made my last commit of the series. I added the missing simplifications and adjusted testing. It is good to go.
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.
@shashi could you take a look?
Merging #418 (d994a48) into master (9bbd946) will decrease coverage by
0.60%
. The diff coverage is48.14%
.
@@ 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%) |
:arrow_down: |
src/utils.jl | 65.00% <100.00%> (+7.25%) |
:arrow_up: |
src/polyform.jl | 93.72% <0.00%> (-0.42%) |
:arrow_down: |
src/ordering.jl | 89.65% <0.00%> (+1.14%) |
:arrow_up: |
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.
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?
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.