Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #48 +/- ##
==========================================
+ Coverage 92.96% 93.11% +0.15%
==========================================
Files 14 14
Lines 2871 2934 +63
==========================================
+ Hits 2669 2732 +63
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds private helper renaming, enhances regex matching, refines simplify logic, introduces a new trigonometric substitution transform, and updates tests and logging.
- Rename
simplify_to_same_standardto_simplify_to_same_standardin test utilities - Extend regex API with
any_constantandcontains, plus related tests - Introduce
TrigUSubtransform for trig substitution and add an integration test - Refine general
simplifylogic and improve debug logger output
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Rename simplify helper to private and update calls |
| tests/test_regex.py | Import and test new any_constant and contains |
| tests/test_khan_academy_integrals.py | Add test for trig substitution |
| src/simpy/transforms.py | Add TrigUSub class and import updated regex API |
| src/simpy/simplify/simplify.py | Fix expand logic and enhance is_simpler |
| src/simpy/regex.py | Update Any_, add any_constant and contains |
| src/simpy/expr.py | Add childless property and tweak nesting logic |
| src/simpy/debug/logger.py | Improve dump header and plot layout |
| benchmark/readme.md | Document logging section in benchmarks |
| benchmark/integration_log.txt | Commit generated log output (should be ignored) |
| benchmark/benchmark-profiler.py | Comment out an ugly expression in the suite |
Comments suppressed due to low confidence (3)
src/simpy/expr.py:335
get_anys(expr2)is called twice in this conditional, which may be expensive; consider caching its result in a local variable.
elif len(expr2.symbols()) == 0 and (len(get_anys(expr2)) == 0 or all([a.is_constant for a in get_anys(expr2)])):
src/simpy/debug/logger.py:73
- The log header is written without a trailing newline, which can cause formatting issues. Consider appending "\n" at the end of the header string.
f.write("Integrand: time taken (s)")
benchmark/integration_log.txt:1
- Committing a generated integration log file to the repository can bloat version control; consider adding it to
.gitignoreor removing it.
Integrand: time taken (s)
| # right now we are doing the sec/tan simplification in a separate function because its implementation is a bit more | ||
| # complex? | ||
| # but i feel like maybe ideally it should be in this function along with cos^2(x) + sin^2(x) = 1 and we can have | ||
| # them both done together with more advanced regex. but for now, this is fine! | ||
| # other_table = [ | ||
| # (r"^sec\((.+)\)\^2$", r"^-tan\((.+)\)\^2$", Const(1)), | ||
| # ] |
There was a problem hiding this comment.
[nitpick] This block of commented-out code and TODO notes can clutter the codebase. Consider cleaning it up or moving it to documentation if still relevant.
| # right now we are doing the sec/tan simplification in a separate function because its implementation is a bit more | |
| # complex? | |
| # but i feel like maybe ideally it should be in this function along with cos^2(x) + sin^2(x) = 1 and we can have | |
| # them both done together with more advanced regex. but for now, this is fine! | |
| # other_table = [ | |
| # (r"^sec\((.+)\)\^2$", r"^-tan\((.+)\)\^2$", Const(1)), | |
| # ] | |
| # Removed unnecessary comment and commented-out code. |
Closes #35
+adds logger output to be committed?