Week 3
Pull Request 2 …. Hello everyone, hope this blog finds you well.
Last week, my primary focus was on successfully submitting Pull Request 1, and I’m glad to share that I was able to accomplish that goal. With that achievement behind me, I promptly shifted my attention towards Pull Request 2. This particular request revolves around the graphical analysis of the control module, requiring the development of various plot-related functions. These functions are crucial for analyzing transfer functions and assessing their stabilities.
Undoubtedly, it has been a challenging week for me. There were moments when my confidence wavered. Currently, I have addressed the comments and feedback received, ensuring that the Pull Request is in the best possible state. It now stands still, awaiting further consideration.
Aim :
My purpose this week was to add the Root_Locus Plot and Nichols Plot to control plots of Sympy-Physics.
I have made the pull request here - #25251.
Work : Let us go through the Pull Request and the reviews I addressed in brief.
- Replacing
Numpy
methods with methods ofSympy Plotting Module
: Myself and Jason had discussed about speed taken by SymPy for plot generation versus the faster speed provided by Numpy. I concluded that although SymPy’s slower performance is expected, it is accepted. Hence I replaced this snippet for logarithmic sampling of points betweeninitial_omega
andfinal_omega
,
From
x = np.linspace(initial_omega, final_omega, 10000)
mag_func = lambdify(_w, mag, modules=['numpy'])
phase_func = lambdify(_w, phase, modules=['numpy'])
mag_points = mag_func(x)
phase_points = phase_func(x)
To
from sympy.plotting.plot import Parametric2DLineSeries
range = (_w,initial_omega,final_omega)
plot_points = Parametric2DLineSeries(phase, mag, range, nb_of_points = 10000, adaptive= False)
phase_points, mag_points = plot_points.get_points()
- Some challenges on this pull request made me discuss my concerns with the SymPy community as follows -
Observations and Challenges :
As of now, the pull request is not progressing in the right direction as the following comments are partially addressed. Here are the blockers I see in addressing these suggestions successfully.
Coloring The Plot
- In Root-Locus plot separate branches are supposed to be colored differently. On following this suggesstion we will have a plot with the same color for every branch which fails the idea of visual clarity and effective analysis.Performance Margins
- This suggestion is not improving the performance by a great extent. It optimizes the sampling of points only on the real axis. If you go through the current code, hardly some points are sampled on the real axis. Most points are sampled on the curve, which would be needed anyways if this suggestion is applied. The minute improvement will be compensated with the additional computation the code would require and also it won’t be of any help when the root-locus plot is not formed on the x axis (all intervals of the parametric equation have imaginary roots).Proof Of Correctness
- The suggestion seems a bit non intuitive and random to implement, I would appreciate if an end to end approach is designed instead of it. These issues make me question if this suggestion is more complicated than it’s worth.
- Lack of required functionalitites in SymPy’s plotting module - I have attempted to replace the Matplotlib plot entirely with SymPy plot. But as of now SymPy cannot link two lists containing points collected using functions sampled on the same set of distributed points. I would not like to enforce use of SymPy unless it is competent enough to provide a level accuracy with respect to MATLAB as provided by Matplotlib.
- As a user in this day and age, speed and responsiveness is an important attribute of any software. Matplotlib plots are better in both these domains compared to pure SymPy plots.
Future Work : In the upcoming week, I would want to come to a conclusion regarding how this Pull Request should be handled. I would recommend merging it in this current state if we cannot improve upon it, since it is clearly a helpful addition. Thanks !