3
\$\begingroup\$

I was studying stochastic processes and thought to independently simulate a moving average process.

# SIMULATING A MOVING AVERAGE PROCESS of order 10
import random
import matplotlib.pyplot as plt

error_list = [random.normalvariate(0, 1) for _ in range(5011)]
ma_process = []

coefficients = [random.randint(-5, 5) for _ in range(11)]
for i in range(10, 5011):
    s = 0
    for j in range(i-10, i):
        # s += error_list[j] # this is for the coefficients all equaling 1
        s += error_list[j] * coefficients[i - j] # coefficients all not 1
    ma_process.append(s)

print(sum(ma_process)/len(ma_process))
plt.plot(ma_process)
plt.axhline(y=0, color='red') # expected value of mean is 0
plt.show()

The print statement is basically to compare the actual value of sample mean with the expected value of 0.

I'd like to know whether I can improve my code using any best practices I may have skipped. I am new to this site but not SE in general, so I have tried to adhere to all the guidelines to asking a good question. Thanks!

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Hi, if you downvote please tell me the reason, so I can improve the post! Thanks! \$\endgroup\$ Dec 2, 2023 at 17:45

1 Answer 1

2
\$\begingroup\$

docstring

# SIMULATING A MOVING AVERAGE PROCESS of order 10

That # comment looks to me like it wants to be a module-level """docstring""".

But then as we read a bit more, it looks like most of the module's code wants to be moved into a def function.


cite your reference

A moving average process is a well known concept. But when you cite a particular author, that gives us more context about the assumptions you're making and the problem you're trying to solve. It admits of more mechanical checking for correctness, by simply drawing a correspondence between the cited work and the source code.

Here, you didn't cite anything, leaving us with the default choice of wikipedia. But if that was your reference, then instead of coefficients the more natural identifier choice would have been theta, so perhaps you had another author in mind.


DRY, magic

error_list = [... for _ in range(5011)]
...
coefficients = [... for _ in range(11)]
for i in range(10, 5011):
    ...
    for j in range(i - 10, i):

Oh, my goodness, so many anonymous magic numbers, some of them repeated. You have passed up the opportunity to give them explanatory names. Some of them appear to be related to each other, in an off-by-one way, but you're not helping the Gentle Reader to understand those relationships.

This code is crying out to be packaged up as a function. And then we could give it keyword-default parameters of ten or five thousand or whatever.

The function should return ma_process.


commented code

Commented code serves a purpose during the edit-debug development cycle, but we remove it prior to requesting PR review, since it shouldn't be merged down to main.

        # s += error_list[j] # this is for the coefficients all equaling 1
        s += error_list[j] * coefficients[i - j] # coefficients all not 1

It's unclear what advice you are trying to offer.

If this loop has two operating modes, then offer a bool parameter which defaults to False, and let the other line of code execute when we've requested the other mode.


output

We see some printing and plotting, once ma_process is available. Package them up in a separate function. This will e.g. allow a unit test to evaluate ma_process figures without producing any chatty output.


This code appears to achieve some of its design goals.

I would not be willing to delegate or accept maintenance tasks on it in its current form.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Wow, this is certainly a huge amount improvement I need to do! I, being a beginner, foolishly believed it to be somewhat good code, but have learnt so much through this answer, especially stuff like magic numbers. Thanks a lot \$\endgroup\$ Dec 5, 2023 at 9:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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