5
\$\begingroup\$

This is my first post, I am very new to coding and Python especially,

This code intends to do an excel SUMIF between two tables with different indexes. The first tables has GPS data with timestamp, vehicle ID and distance The second table has vehicle ID and timestamps of events I want to measure the distance run during events

Thanks

for x in range(1,34):
df = pd.read_csv("file"
                 + str(x) + '.csv',
                 parse_dates=[10])

red = 0
green = 0
black = 0

output = [[], [], [], []]
for i in range(len(lista[1])):
    for j in range(len(listc[1])):
        if listc[1][j] <= lista[3][i] or listc[1][j] >= lista[2][i]:
            if lista[7][i] >= listc[1][j] and lista[6][i] <= listc[1][j] and lista[0][i] == listc[0][j] and lista[8][i] == 'intended value' :
                red += listc[2][i]
            if lista[3][i] >= listc[1][j] and lista[7][i] <= listc[1][j] and lista[0][i] == listc[0][j] and lista[8][i] != 'intended value' :
                red += listc[2][i]
            if lista[6][i] >= listc[1][j] and lista[2][i] <= listc[1][j] and lista[0][i] == listc[0][j] and lista[8][i] == 'intended value' :
                green += listc[2][i]
            if lista[7][i] >= listc[1][j] and lista[2][i] <= listc[1][j] and lista[0][i] == listc[0][j] and lista[8][i] != 'intended value' :
                green += listc[2][i]
            if lista[2][i] >= listc[1][j] and lista[3][i - 1] <= listc[1][j] and lista[0][i] == listc[0][j]:
                black += listc[2][i]
    toc = timeit.default_timer()
    if i % 100 == 0:
        print('processing algorithm: {}'.format(toc - tic))
        print('we are at row {}'.format(i))
    output[0].append(lista[1][i])
    output[1].append(red)
    output[2].append(green)
    output[3].append(black)
    red = 0
    green = 0
    black = 0
toc = timeit.default_timer()
np.savetxt("outfile" + str(x)
           + ".csv", np.column_stack((output[0], output[1], output[2], output[3])), delimiter=",", fmt='%s')
tac = timeit.default_timer()
print('exporting {}'.format(tac - toc))
\$\endgroup\$
6
  • 7
    \$\begingroup\$ Welcome to code review, as this looks very ambiguous, I suggest you explain what is the purpose of this code and you might provide examples of sample input and desired output as well as the csv files you're working with to make it easier for people to understand what is a wrong/right output and review your code. \$\endgroup\$
    – watch-this
    Jan 9, 2020 at 7:17
  • 3
    \$\begingroup\$ Could you prodie a Minimum Working Exemple ? For the moment, without the data, i'm unable to run your code and hence i cant tests eventual improvements. \$\endgroup\$
    – lrnv
    Jan 9, 2020 at 8:25
  • 1
    \$\begingroup\$ Please fix the block nesting/indentation of the first for-loop. \$\endgroup\$
    – greybeard
    Jan 9, 2020 at 8:45
  • 3
    \$\begingroup\$ Your edit is a decent improvement - for the motivation part. Please roll back the changes in the code, see What should I not do when someone answers my question? (You are welcome to ask a cross-linked follow-up question.) \$\endgroup\$
    – greybeard
    Jan 9, 2020 at 14:51
  • 1
    \$\begingroup\$ Welcome to Code Review! Please edit your question so that the title describes the purpose of the code, rather than its mechanism. We really need to understand the motivational context to give good reviews. In particular, if "bad performance" isn't part of the requirement, then it probably doesn't belong in the title. Thanks! \$\endgroup\$ Jan 10, 2020 at 14:42

4 Answers 4

7
\$\begingroup\$

For me, the problem starts with the nested loops showing no specification of what is to be achieved, not even a suggested abstraction (being the body of a function given a name).

Observations:

  • the output does seem to depend on the order of elements of lista
    (even beyond its order: lista[3][i-1])
    • hope lista[2][i] >= listc[1][j] is never True for i 0
     (unless you want lista[3][-1] accessed)
  • the output does not seem to depend on the order of elements of listc
  • both lista and listc are not changed
    → the "range conditions" won't change unless at least one index changes
  • all of "the increments" share the condition lista[0][i] == listc[0][j]
  • the conditions between lista[6/7][i] and listc[1][j] are not complementary for including equality in both cases
    • implying red/green possibly getting incremented twice in a single iteration (not using else)

idea:

  • document, in the code, what is to be achieved
    Python supports this with docstrings
  • use telling names
  • have a tool help you sticking to The Python Style Guide
  • order listc
  • for each i, iterate only that part of the ordered listc where lista[0][i] == listc[0][j]
  • ignore if lista and listc are not "rectangular":

food for thought: untested result of refactoring (get tool support for such, too)
(here extracting local variables, mostly)

list_c = sorted(listc)
for i in range(len(lista[1])):
    red = green = black = 0
    a0i = lista[0][i]
    first = bisect_left(list_c[1], a0i)
    beyond = bisect_right(list_c[1], a0i, first)
    if first < beyond:
        a2i = lista[2][i]
        a3i = lista[3][i]
        c2i = list_c[2][i]
        a8i_intended = lista[8][i] == 'intended value'
        for j in range(first, beyond):
            c1j = list_c[1][j]
            if (c1j <= a3i or c1j >= a2i):
                if lista[7][i] >= c1j and lista[6][i] <= c1j and a8i_intended:
                    red += c2i
                if a3i >= c1j and lista[7][i] <= c1j and not a8i_intended:
                    red += c2i
                if lista[6][i] >= c1j and a2i <= c1j and a8i_intended:
                    green += c2i
                if lista[7][i] >= c1j and a2i <= c1j and not a8i_intended:
                    green += c2i
                if a2i >= c1j and lista[3][i - 1] <= c1j:
                    black += c2i
    toc = timeit.default_timer()
    if i % 100 == 0:
        print('processing algorithm: {}'.format(toc - tic))
        print('we are at row {}'.format(i))
    output[0].append(lista[1][i])
    output[1].append(red)
    output[2].append(green)
    output[3].append(black)

afterthought: it may be better to handle listc[1][j] <= lista[3][i] and lista[2][i] <= listc[1][j] separately

\$\endgroup\$
6
\$\begingroup\$

The code does not look appetizing, readable.

I reduced the conditionals which indeed brought some structure into the whole:

red = 0
green = 0
black = 0

    c1 = listc[1][j]
    if c1 <= lista[3][i] or c1 >= lista[2][i]:
        if lista[0][i] == listc[0][j]:
            c2 = listc[2][i]
            if lista[8][i] == 'intended value':
                if lista[6][i] <= c1 <= lista[7][i]:
                    red += c2
                if lista[2][i] <= c1 <= lista[6][i]:
                    green += c2
            else:
                if lista[7][i] <= c1 <= lista[3][i]:
                    red += c2
                if lista[2][i] <= c1 <= lista[7][i]:
                    green += c2
            if lista[3][i - 1] <= c1 <= lista[2][i]:
                black += c2

The variables red, green, black to be initialized at the start of the for-i step.

Notice the between expression ... <= ... <= ..., a pearl in the Python language.

Introducing variables, especially with good names enormously helps in reading, and simplifies all. Unfortunately here it does not seem to work for indices 6, 7, 2, 6 etcetera.

The algorithm could have been smaller, without repetitive [i] and [j], when one would not have lista and listc with [column][row] but [row][column]. That is not doable without altering too much.

But one could make columns with meaningful names (not lista3):

lista3 = lista[3]
...
\$\endgroup\$
2
  • \$\begingroup\$ I missed the pairs of comparisons constituting "betweens" for all the indices and identifiers floating around. \$\endgroup\$
    – greybeard
    Jan 9, 2020 at 13:36
  • 2
    \$\begingroup\$ The between expression is sadly not vectorised in numpy. But ~,| and & are ! \$\endgroup\$
    – lrnv
    Jan 9, 2020 at 13:38
2
\$\begingroup\$

Working from @JoopEgen answer, i wrote a numpy version that will usualy speed up the whole thing by a huge factor (but since no data are given, i cant test it...)

Well, while doing it, i remarked that you use :

for i in range(len(lista[1])):
    ...
    lista[1][i-1]

which is wierd. I then consider that you intended that the last value will be used as the first, as a previous comment proposed. Anyway here is a probably faster version :

import numpy as np

# Rename all this and make them numpy arrays to profit from broadcasting :
x = [np.array(lista[n]) for n in [1,2,3,6,7]] # becomes 0,1,2,3,4
x.append(np.array(lista[8]) == 'intended value') # 5
x.append(np.array(listc[0])) # 6
x.append(np.array(listc[1])) # 7
x.append(x[0]) # 8
for j in np.arange(len(lista[1])):
    x[8][j] = lista[3,j-1] # the shifted values for the last conditions.

# the final values for the output :
val = np.array(listc[2])

# Selectors :
common = (x[1] == x[6]) & ((x[7] <= x[2]) | (x[7] >= x[1]))
red = common & ((x[3] <= x[7]) & (x[7] <= x[4]) & x[5]) | ((x[4] <= x[7]) & (x[7] <= x[2]) & (~x[5]))
gre = common & ((x[1] <= x[7]) & (x[7] <= x[3]) & x[5]) | ((x[1] <= x[7]) & (x[7] <= x[4]) & (~x[5]))
bla = common & ( x[8] <= x[7]) & (x[7] <= x[1])

# the result :
output = np.array([val,val[reds],val[greens],val[blacks]])
\$\endgroup\$
0
\$\begingroup\$

After reviewing some of the answers I rewrited the code and added some descriptions This does not work because I have a index error in pandas...

'''

This code intends to do an excel SUMIF between two tables with different indexes. The first tables has GPS data with timestamp, vehicle ID and distance The second table has vehicle ID and timestamps of events I want to measure the distance run during events

Initially I tried to join the tables (dataframes) somehow while working with pandas but I failed After that I made them lists

'''

import pandas as pd
from datetime import datetime
import xlrd
import numpy as np
import timeit

tic = timeit.default_timer()

dfRaw = pd.read_excel("C:\\Users\\pavlo\\PycharmProjects\\PEXproject1\\DataCleaning\\sample data\\VehicleEvents.xlsx", sheet_name="vhcllist") #reading the vehicle event list
dfRaw = dfRaw.fillna(2000, inplace=False) #replacing the NaN values with 2000 to avoid datetime errors

book = xlrd.open_workbook("C:\\Users\\pavlo\\PycharmProjects\\PEXproject1\\DataCleaning\\sample data\\VehicleEvents.xlsx") #I re-open the file, not sure why...
datemode = book.datemode

dfRaw["Engineon"].map(lambda x: # Because the date-times in xls were saved with the excel float format, I found this way to make it into datetime
          xlrd.xldate_as_tuple(x, datemode))
dfRaw["Engineoff"].map(lambda x:
          xlrd.xldate_as_tuple(x, datemode))
dfRaw["WorkStart"].map(lambda x:
          xlrd.xldate_as_tuple(x, datemode))
dfRaw["WorkEnd"].map(lambda x:
          xlrd.xldate_as_tuple(x, datemode))
dfRaw["ParkStart"].map(lambda x:
          xlrd.xldate_as_tuple(x, datemode))
dfRaw["ParkEnd"].map(lambda x:
          xlrd.xldate_as_tuple(x, datemode))

dfRaw["ENGINEON"] = dfRaw["Engineon"].map(lambda x: # I made new columns in the dataframe because I had trouble updating the current ones
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))
dfRaw["ENGINEOFF"] = dfRaw["Engineoff"].map(lambda x:
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))
dfRaw["WORKSTART"] = dfRaw["WorkStart"].map(lambda x:
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))
dfRaw["WORKEND"] = dfRaw["WorkEnd"].map(lambda x:
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))
dfRaw["PARKSTART"] = dfRaw["ParkStart"].map(lambda x:
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))
dfRaw["PARKEND"] = dfRaw["ParkEnd"].map(lambda x:
          datetime(*xlrd.xldate_as_tuple(x,
                                                  datemode)))

dfRaw['TMP'] = dfRaw['ID']
dfRaw = dfRaw.drop('ID', axis=1)
dfRaw['ID'] = dfRaw['Vhcl']

templist = dfRaw[['ID', 'TMP',                                  # I make the dataframe into a temp list
             'ENGINEON', 'ENGINEOFF', 'WORKSTART',
             'WORKEND', 'PARKSTART', 'PARKEND', 'Mode', 'Vhcl']]

vehiclist = [1, 2, 3, 4, 5, 6, 7, 8, 9]                         # Now it is a list of lists with the indexes I need
vehiclist[0] = templist['ID'].tolist()
vehiclist[1] = templist['TMP'].tolist()
vehiclist[2] = templist['ENGINEON'].tolist()
vehiclist[3] = templist['ENGINEOFF'].tolist()
vehiclist[4] = templist['WORKSTART'].tolist()
vehiclist[5] = templist['WORKEND'].tolist()
vehiclist[6] = templist['PARKSTART'].tolist()
vehiclist[7] = templist['PARKEND'].tolist()
vehiclist[8] = templist['Mode'].tolist()

for x in range(1,34): # here the code will read from 34 csv files containing GPS informations into a dataframe
    df = pd.read_csv("C:\\Users\\pavlo\\PycharmProjects\\PEXproject1\\DataCleaning\\sample data\\GpsData"
                 + str(x) + '.csv',
                 parse_dates=[10])

    df['ID'] = df['gps_id']

    gps = df[['ID','Timestamp','distance']] # here I copy the data from the dataframe to a list
    gpslist = [1,2,3]                           # I make the list of lists
    gpslist[0] = gps['ID'].tolist()
    gpslist[1] = gps['Timestamp'].tolist()
    gpslist[2] = gps['distance'].tolist()

    driving = 0
    idle = 0
    working = 0

    dists = [[], [], [], []]                    #this list of lists will capture the distances in the various states
    for i in range(len(vehiclist[1])):          #I go through all rows of vehicle list
        driving = idle = working = 0
        for j in range(len(gps[1])):            #I go through all rows of gps list
            if gps[1][j] <= vehiclist[3][i] or gps[1][j] >= vehiclist[2][i]:   #I want to exclude if the vehicle was off at the gps timestamp
                if vehiclist[0][i] == gps[0][j]:
                    c1 = gps[2][i]
                    c2 = gps[1][j]
                    if vehiclist[8][i] == 'Manual' :
                        if vehiclist[6][i] <=  c1 <= vehiclist[7][i] :
                            driving += c2
                        if vehiclist[2][i] <= c1  <= vehiclist[6][i] :
                            idle += c2
                    else:
                        if vehiclist[7][i] <= c1 <= vehiclist[3][i] :
                            driving += c2
                        if vehiclist[2][i] <= c1 <= vehiclist[7][i] :
                            idle += c2
                    if vehiclist[3][i] <= c1 <= vehiclist[2][i - 1] :
                            working += c2
        toc = timeit.default_timer()
        if i % 100 == 0:
            print('processing algorithm: {}'.format(toc - tic))
            print('we are at row {}'.format(i))
        dists[0].append(vehiclist[1][i])
        dists[1].append(driving)
        dists[2].append(idle)
        dists[3].append(working)
        driving = 0
        idle = 0
        working = 0
    toc = timeit.default_timer()
    np.savetxt("outfile" + str(x)
               + ".csv", np.column_stack((dists[0], dists[1], dists[2], dists[3])), delimiter=",", fmt='%s')
    tac = timeit.default_timer()
    print('exporting {}'.format(tac - toc))
\$\endgroup\$

Your Answer

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

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