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
Feedback will now act like a TransferFunction #26228
Feedback will now act like a TransferFunction #26228
Conversation
✅ Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes. Your release notes are in good order. Here is what the release notes will look like:
This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
@faze-geek can you please check this PR, and suggest future work here ? |
I've changed the structure of the code such that the |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) | Change | Before [a00718ba] | After [ab2fb691] | Ratio | Benchmark (Parameter) |
|----------|----------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 67.1±0.5ms | 44.3±0.2ms | 0.66 | integrate.TimeIntegrationRisch02.time_doit(10) |
| - | 67.1±0.9ms | 43.5±0.2ms | 0.65 | integrate.TimeIntegrationRisch02.time_doit_risch(10) |
| + | 18.6±0.2μs | 29.9±0.3μs | 1.61 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 5.24±0.02ms | 2.85±0.04ms | 0.54 | logic.LogicSuite.time_load_file |
| - | 72.8±0.3ms | 28.5±0.2ms | 0.39 | polys.TimeGCD_GaussInt.time_op(1, 'dense') |
| - | 25.7±0.05ms | 16.7±0.07ms | 0.65 | polys.TimeGCD_GaussInt.time_op(1, 'expr') |
| - | 72.2±0.4ms | 28.6±0.1ms | 0.4 | polys.TimeGCD_GaussInt.time_op(1, 'sparse') |
| - | 253±2ms | 124±0.5ms | 0.49 | polys.TimeGCD_GaussInt.time_op(2, 'dense') |
| - | 254±1ms | 123±1ms | 0.48 | polys.TimeGCD_GaussInt.time_op(2, 'sparse') |
| - | 654±5ms | 369±2ms | 0.56 | polys.TimeGCD_GaussInt.time_op(3, 'dense') |
| - | 656±2ms | 368±2ms | 0.56 | polys.TimeGCD_GaussInt.time_op(3, 'sparse') |
| - | 485±2μs | 286±1μs | 0.59 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense') |
| - | 1.78±0.01ms | 1.03±0.01ms | 0.58 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense') |
| - | 5.71±0.02ms | 3.04±0.02ms | 0.53 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 448±8μs | 228±1μs | 0.51 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense') |
| - | 1.46±0.01ms | 667±5μs | 0.46 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense') |
| - | 4.87±0.01ms | 1.66±0.01ms | 0.34 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 370±2μs | 206±0.4μs | 0.56 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 2.43±0.02ms | 1.21±0.01ms | 0.5 | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 9.94±0.04ms | 4.33±0.02ms | 0.44 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 356±2μs | 169±1μs | 0.47 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense') |
| - | 2.50±0.01ms | 898±5μs | 0.36 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 9.47±0.08ms | 2.66±0.01ms | 0.28 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 1.03±0.01ms | 431±4μs | 0.42 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 1.79±0.06ms | 516±1μs | 0.29 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.86±0.05ms | 1.80±0.02ms | 0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense') |
| - | 8.28±0.1ms | 1.52±0ms | 0.18 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 284±0.4μs | 66.5±0.7μs | 0.23 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 3.41±0.02ms | 391±3μs | 0.11 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 3.96±0.01ms | 291±3μs | 0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 7.09±0.08ms | 1.27±0.01ms | 0.18 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense') |
| - | 8.64±0.03ms | 860±5μs | 0.1 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 5.02±0.03ms | 2.98±0.01ms | 0.59 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 11.9±0.08ms | 6.55±0.03ms | 0.55 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense') |
| - | 22.2±0.2ms | 9.05±0.02ms | 0.41 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 5.25±0.02ms | 873±3μs | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 12.7±0.09ms | 7.03±0.02ms | 0.55 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 102±0.4ms | 25.4±0.05ms | 0.25 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense') |
| - | 165±1ms | 53.6±0.1ms | 0.32 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 173±1μs | 111±2μs | 0.64 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense') |
| - | 356±1μs | 219±0.8μs | 0.61 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse') |
| - | 4.26±0.04ms | 845±4μs | 0.2 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense') |
| - | 5.21±0.03ms | 384±1μs | 0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 19.9±0.1ms | 2.78±0.01ms | 0.14 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense') |
| - | 22.5±0.1ms | 630±7μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 476±2μs | 138±1μs | 0.29 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 4.70±0.05ms | 623±2μs | 0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense') |
| - | 5.22±0.04ms | 140±0.9μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 13.0±0.1ms | 1.28±0ms | 0.1 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense') |
| - | 13.8±0.06ms | 141±0.9μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 133±0.6μs | 76.0±0.5μs | 0.57 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 250±1μs | 88.7±0.4μs | 0.36 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 24.4±0.2ms | 10.1±0.05ms | 0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
| - | 28.5±0.08ms | 15.4±0.1ms | 0.54 | solve.TimeSparseSystem.time_linsolve_Aaug(20) |
| - | 55.1±0.4ms | 24.6±0.08ms | 0.45 | solve.TimeSparseSystem.time_linsolve_Aaug(30) |
| - | 28.4±0.4ms | 15.2±0.07ms | 0.53 | solve.TimeSparseSystem.time_linsolve_Ab(20) |
| - | 54.2±0.2ms | 24.4±0.1ms | 0.45 | solve.TimeSparseSystem.time_linsolve_Ab(30) |
Full benchmark results can be found as artifacts in GitHub Actions |
Changing both |
I'm not fully convinced about the design decision we would take with |
I have not reviewed any changes to |
8e69eed
to
8658ae2
Compare
It's good practice to give unique commit names relevant to the changes in that commit. Keep that in mind for future. |
Thank you for the guidance! I'll make sure to provide more descriptive commit names in the future. |
@faze-geek @moorepants Sir, can you please review this PR? So that I could also work on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks okay to me, I'd wait for others to make sure that the design is perfect before merging.
# Combining Operations of TransferFunction and Feedback | ||
fd2 = Feedback(tf1*fd1, tf2) | ||
assert fd2.doit().simplify() == TransferFunction(2*(s + 1)**2, 2*s + ((s + 1)*(s + 2) + 1)*(s**2 + 2*s + 1) + 2, s) | ||
assert (tf1+fd1) == Parallel(TransferFunction(2*s + 2, s**2 + 2*s + 1, s), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, just for proof of correctness at my end can you point me to any links where these tests have been picked up from. I usually added MATLAB examples as my unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohk, Sir I'll include the tests from MATLAB or other genuine sources and Update the tests by today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's okay if you don't find relevant MATLAB examples, I would still try to use other online or textbook examples rather than making up tests on my own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have gone over the tests, they look good to me!
I will merge this by tomorrow EOD if reviewers don't have any other concerns. |
Merging. Thanks for your contributions. |
I see this was merged, but I still don't understand why we are calling Feedback should look like: f = Feedback(tf1, tf2)
f.num == tf1
f.dem == Parallel(1, Series(tf1, tf2)) no? |
Sir, In this review comment it was said that the |
@faze-geek we can either automatically evaluate these transfer function or not. If we do the former, you might as well just work with an expression and there is no need for a TransferFunction object, right? I don't quite understand the design decisions. A sympy expression is a tree, for example
The user never sees this and the printer displays We originally designed |
I actually was thinking that using |
It isn't that is wrong, but we do all need to understand what the design is and it needs to be written down in the docs so that people follow the design pattern. |
Sir, I've understood the design and requirements, and I'm currently working on this. |
References to other Issues or PRs
Brief description of what is fixed or changed
Feedback will now act like a TransferFunction.
Solves Issue #26161
Other comments
Release Notes