Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have written a piece of code (in Py3) that graphs out the controller value of PID controller. This is so that by changing the various sub controllers (P/I/D) I can visualise how the graph changes. I've written the following code, but having difficulty in the PID algorithm, and obtaining a correct controller value. You can run the code as is, and it should output a graph. The code works correctly as intended.

 #!/usr/bin/python3

import random
from decimal import *
import matplotlib
matplotlib.use('Agg')
import matplotlib.pyplot as plt
import pandas as pd
import numpy as np
import os

# SOURCE: http://blog.nikmartin.com/2012/11/process-control-for-dummies-like-me.html

y_data = []
x_data = []
error_value_data = []
current_value_data = []
controller_output_data = []
proportional_value_data = []
integral_value_data = []
derivative_value_data = []
setpoint_value_data = []
process_value_data = []

OUTGOING_FILES_DIRECTORY = "/some/dir/in/your/computer"

def create_offset(setpoint_value):
    offset_value = random.randint(-128, 128)
    offset_value_incrementation = float(offset_value / 100)
    return setpoint_value - offset_value_incrementation


def Pt_controller(process_error_value, Pgain_value):
    Pt = process_error_value * Pgain_value
    return Pt


def It_controller(It, process_error_value, Igain_value):
    It = It + process_error_value
    It = It * Igain_value
    return It


def Dt_controller(current_control_output_value, previous_control_output_value, derivative_gain_value):
    Dt = (current_control_output_value - previous_control_output_value) * derivative_gain_value
    # print(current_control_output_value, previous_control_output_value, Dt)
    return Dt

def process_error(setpoint_value, current_value):
    return setpoint_value - current_value

def pid_controller(proportional_value, integral_value, derivative_value):
    pid_output = derivative_value + (proportional_value + integral_value)
    return pid_output

def graph_line_data(x_value, error_value, proportional_value, integral_value, derivative_value, setpoint_value, process_value, PID_value):

    if max(x_value) > 500:
        width = max(x_value)/200
    else:
        width = 10
    height = 5
    plt.figure(figsize=(width, height), dpi=100)

    # Choose which data
    plt.plot(x_value, PID_value, '#FF4500')
    # plt.plot(x_value, error_value, 'r')
    # plt.plot(x_value, proportional_value, '#9F00A0')
    # plt.plot(x_value, integral_value, 'green')
    # plt.plot(x_value, derivative_value, '#00BBFF')
    plt.plot(x_value, setpoint_value, 'k--')
    # plt.plot(x_value, process_value, 'b')

    plt.xlabel('Iteration Count')
    plt.ylabel('Output')
    plt.title('PID Controller\n(P=0.82, I=0.155, D=0.45)')

    plt.legend(['PID Controller Output', 'Set point'], loc='upper right')
    # plt.legend(['PID Controller Output'], loc='upper right')

    # plt.legend(['PID Controller Output', 'Error Value', 'Integral Value'], loc='upper right')

    # plt.legend(['PID', 'Error Value', 'Proportional Value', 'Integral Value',
    #            'Derivative Value', 'Set point', 'Process Value'], loc='upper right')

    png_filename = "Steady_PID_Simulation"

    output_file_path = os.path.join(OUTGOING_FILES_DIRECTORY, png_filename)

    plt.savefig(output_file_path)

    print("Finished creating: " + png_filename + ".png")




def pid_simulator():
    # set point
    setpoint_value = 0

    # gain values          # P = 0.82 / I = 0.15 / D = 0.40
    proportional_gain_value = 0.82
    integral_gain_value = 0.155
    derivative_gain_value = 0.45

    # initialisation values
    derivative_value = 1
    controller_output_value = 0
    integral_value = 0
    x_value = 0
    number_of_iterations = 200

    while x_value < number_of_iterations:
        x_value += 1

        process_value = setpoint_value

        error_value = process_value - setpoint_value

        if x_value == number_of_iterations / 5:
            setpoint_value = 100
        if x_value == number_of_iterations / 2:
            setpoint_value = -300
        if x_value == 6 * (number_of_iterations / 10):
            setpoint_value = -200
        if x_value == 7 * (number_of_iterations / 10):
            setpoint_value = -250
        if x_value == (number_of_iterations - (number_of_iterations / 5)):
            setpoint_value = 0

        # PROPORTIONAL
        proportional_value = Pt_controller(process_value, proportional_gain_value)

        # INTEGRAL
        integral_value = It_controller(integral_value, process_value, integral_gain_value)

        # CONTROLLER OUTPUT
        previous_controller_output_value = controller_output_value
        controller_output_value = proportional_value + integral_value + derivative_value

        # DERIVATIVE
        derivative_value = Dt_controller(controller_output_value, previous_controller_output_value, derivative_gain_value)
        # derivative_value = 0.25 * derivative_value

        # print(integral_value)
        # print(number_of_iterations - x_value)
        x_data.append(x_value)
        controller_output_data.append(controller_output_value)
        error_value_data.append(error_value)
        integral_value_data.append(integral_value)
        setpoint_value_data.append(setpoint_value)
        process_value_data.append(process_value)
        derivative_value_data.append(derivative_value)
        proportional_value_data.append(proportional_value)

    graph_line_data(x_data, error_value_data, proportional_value_data, integral_value_data, derivative_value_data,
                    setpoint_value_data, process_value_data, controller_output_data)




def main():
    pid_simulator()



if __name__ == "__main__":
    main()

The output yields: Steady PID output

share|improve this question

closed as off-topic by ferada, forsvarir, t3chb0t, Peilonrayz, Graipher Feb 15 at 15:49

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – ferada, forsvarir, t3chb0t, Peilonrayz, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

1  
Welcome to StackExchange Code Review! On this site we review working code. Suggest you take a look at these Guidelines – Stephen Rauch Feb 13 at 5:17
1  
Don't seem to follow what's off topic Especially when it's on hold as "broken/not yet written" The code is clearly fully complete, but there is something going wrong, I just want someone from "StackExchange [to] Code Review" the code above. – 3kstc Feb 13 at 5:24
7  
If it is not working correctly as intended, then we consider it to be broken code and hence off-topic for Code Review. – 200_success Feb 13 at 7:05
    
Thanks for rewording the question in Rev 2. On Code Review, it is common practice to put apparently off-topic questions on hold as early as possible, to ensure that any broken code is corrected before anyone reviews it, since we don't allow the code to be changed once an answer has been posted. – 200_success Feb 13 at 12:52
6  
@3kstc bear with apparently off-topic questions from this comment. Nobody put your question off-topic without a reason. The way you asked your question ( (...) but having difficulty in the PID algorithm, and obtaining a correct controller value (...) ) was ambiguous. I hope you get some good answers :) – Dex' ter Feb 13 at 13:26

I want to start this review by explicitly saying that I don't have a deep knowledge when it comes to matplotlib but I'll give it a try anyway


Have in mind that I just reviewed the code as is, keeping the same logic / functionality it has at the moment.


Unused imports

Don't import modules you're not using (decimal, pandas, numpy).

Imports grouping

From PEP8 which is the official Python style guide:

Imports should be grouped in the following order:

  1. standard library imports
  2. related third party imports
  3. local application/library specific imports

Refactored imports

import os
import random

import matplotlib
import matplotlib.pyplot as plt

Spacing (a small aspect making a huge difference)

  • you should have two new lines after each method

    def method_1()
        ...
    
    
    def method_2()
        ...
    
  • use the function naming rules: lowercase with words separated by underscores as necessary to improve readability. Pt_controller() should be pt_controller()

  • the same rule as above can (and should) also be applied to variable names

  • try to stick with a max length of maximum 120 characters

About the code

  • try not to create variables which are used only once. Instead you can directly return the result.
  • don't use global variables if it's not necessary (more, don't create useless variables unless you're using them: e.g y_data and current_value_data list are never used)
  • don't abuse comments. Instead, there're the docstrings. More, debug comments are good, but make sure you take them out (I did this)
  • you used if __name__ == "__main__": which is very good !
  • you have some magic numbers which I'd move at the top of the program so that you can easily change them if necessary

Revised code:

import os
import random

import matplotlib
import matplotlib.pyplot as plt

PNG_FILENAME = "Steady_PID_Simulation1.png"
OUTGOING_FILES_DIRECTORY = "C:\\Main\\Projects\\Python\\untitled"
OUTPUT_FILE_PATH = os.path.join(OUTGOING_FILES_DIRECTORY, PNG_FILENAME)

DPI = 100
MAX_VALUE = 500
PLOT_COLOR = '#FF4500'

PROPORTIONAL_GAIN_VALUE = 0.82
INTEGRAL_GAIN_VALUE = 0.155
DERIVATIVE_GAIN_VALUE = 0.45


def create_offset(set_point_value):
    """Docstring here (what does the function do)"""
    offset_value = random.randint(-128, 128)
    offset_value_incrementation = float(offset_value / 100)

    return set_point_value - offset_value_incrementation


def pt_controller(process_error_value, p_gain_value):
    """Docstring here (what does the function do)"""
    return process_error_value * p_gain_value


def it_controller(it, process_error_value, i_gain_value):
    """Docstring here (what does the function do)"""
    it = it + process_error_value
    it = it * i_gain_value
    return it


def dt_controller(current_control_output_value, previous_control_output_value, derivative_gain_value):
    """Docstring here (what does the function do)"""
    return (current_control_output_value - previous_control_output_value) * derivative_gain_value


def process_error(set_point_value, current_value):
    """Docstring here (what does the function do)"""
    return set_point_value - current_value


def pid_controller(proportional_value, integral_value, derivative_value):
    """Docstring here (what does the function do)"""
    return derivative_value + (proportional_value + integral_value)


def get_width_and_height(x_value, weight=10, height=5):
    """Docstring here (what does the function do)"""
    return (max(x_value)/200, height) if max(x_value) > MAX_VALUE else (weight, height)


def graph_line_data(x_value, error_value, proportional_value, integral_value, derivative_value, setpoint_value,
                    process_value, PID_value):
    """Docstring here (what does the function do)"""

    width, height = get_width_and_height(x_value)

    plt.figure(figsize=(width, height), dpi=DPI)

    plt.plot(x_value, PID_value, PLOT_COLOR)
    plt.plot(x_value, setpoint_value, 'k--')

    plt.xlabel('Iteration Count')
    plt.ylabel('Output')
    plt.title('PID Controller\n(P={}, I={}, D={})'.format(PROPORTIONAL_GAIN_VALUE,
                                                          INTEGRAL_GAIN_VALUE,
                                                          DERIVATIVE_GAIN_VALUE))

    plt.legend(['PID Controller Output', 'Set point'], loc='upper right')
    plt.savefig(OUTPUT_FILE_PATH)

    print("Finished creating: {}".format(PNG_FILENAME))


def pid_simulator():
    """Docstring here (what does the function do)"""
    set_point_value, controller_output_value, integral_value, derivative_value, number_of_iterations = 0, 0, 0, 1, 200

    x_data, error_value_data, controller_output_data, proportional_value_data = [], [], [], []
    integral_value_data, derivative_value_data, setpoint_value_data, process_value_data = [], [], [], []

    for x_value, _ in enumerate(range(number_of_iterations)):
        process_value = set_point_value
        error_value = process_value - set_point_value

        if x_value == number_of_iterations / 5:
            set_point_value = 100
        if x_value == number_of_iterations / 2:
            set_point_value = -300
        if x_value == 6 * (number_of_iterations / 10):
            set_point_value = -200
        if x_value == 7 * (number_of_iterations / 10):
            set_point_value = -250
        if x_value == (number_of_iterations - (number_of_iterations / 5)):
            set_point_value = 0

        proportional_value = pt_controller(process_value, PROPORTIONAL_GAIN_VALUE)
        integral_value = it_controller(integral_value, process_value, INTEGRAL_GAIN_VALUE)

        previous_controller_output_value = controller_output_value
        controller_output_value = proportional_value + integral_value + derivative_value

        derivative_value = dt_controller(controller_output_value,
                                         previous_controller_output_value,
                                         DERIVATIVE_GAIN_VALUE)

        x_data.append(x_value)
        controller_output_data.append(controller_output_value)
        error_value_data.append(error_value)
        integral_value_data.append(integral_value)
        setpoint_value_data.append(set_point_value)
        process_value_data.append(process_value)
        derivative_value_data.append(derivative_value)
        proportional_value_data.append(proportional_value)

    graph_line_data(x_data, error_value_data, proportional_value_data, integral_value_data, derivative_value_data,
                    setpoint_value_data, process_value_data, controller_output_data)


def main():
    matplotlib.use('Agg', warn=False)

    pid_simulator()


if __name__ == "__main__":
    main()

I'm sure pid_simulator() can be improved even more but that's all the time that I have with me right now.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.