Ytukyg

I have a requirement to write a Python 3.6 script which should be able to detect following case within a time period and be able to create a text file report that lists the condition violated and transaction that caused the violation:

• Redeeming over 3 time on same stamp card within 10 mins

The required table in database for storing user and stamp card details has following structure-

Here the 'to_sate' column with values 2,3 and 4 means a card is redeemed.

Now, I have written following script to fulfill above requirement and it is working as expected. In my local environment it is taking 2.7 minutes to complete the process for 36000 records-

def insert_data(csv_import_path, csv_export_path):

    import time

    from multiprocessing import Pool

    import multiprocessing

    import pandas as pd

    import pymysql

    # Connect to the database

    engine = pymysql.connect(host='localhost',

                             user='admin',

                             password='MY_PASSWORD',

                             db='MY_DB',

                             charset='utf8mb4',

                             cursorclass=pymysql.cursors.DictCursor)

    df = pd.read_sql(

        "SELECT stamps_record_id, user_id,  stamp_card_id, stamp_time, merchant_id, merchant_store_id FROM rmsdb.user_stamping_records where to_state in (2,3,4) order by stamp_card_id",

        engine)

    df.to_csv(csv_import_path)

    df = pd.read_csv(csv_import_path, index_col=["stamps_record_id"])

    unique_users = df.user_id.unique()

    df["stamp_time"] = pd.to_datetime(df["stamp_time"])

    num_processes = multiprocessing.cpu_count()

    s_time = time.time()

    with Pool(num_processes) as p:

        final_df = pd.DataFrame()

        for i in range(0, len(unique_users)):

            user = unique_users[i]

            new_df = df[df.user_id == user]

            sid = new_df.stamp_card_id.unique()

            for i in sid:

                fdf = new_df[new_df.stamp_card_id == i]

                # len(fdf) can be user given value

                if len(fdf) > 3:

                    for i in range(0, len(fdf)):

                        g = (fdf.iloc[i:i + 3])

                        if len(g) >= 3:

                            x = (g.tail(1).stamp_time.values - g.head(1).stamp_time.values).astype("timedelta64[s]")

                            if x[0].astype(int) < 600:

                                final_df = final_df.append(g)



    e_time = time.time() - s_time

    #     final_df.drop_duplicates(keep="first").to_csv("C:\Users\rahul.khanna\Desktop\user_stamping_records_frauds.csv", index=False)

    final_df.drop_duplicates(keep="first").to_csv(csv_export_path, index=False)



    print("Total Time taken is: " + str(e_time / 60) + " minutes.")



if __name__ == '__main__':

    insert_data("C:\Users\hitman\Desktop\user_stamping_records_import.csv", "C:\Users\hitman\Desktop\user_stamping_records_frauds.csv")

I have converted the df to dictionary for a sample-

34198: '2018-10-13 16:48:03', 34199: '2018-10-13 16:48:03', 34200: '2018-10-13 16:48:03', 34201: '2018-10-13 16:48:03', 34202: '2018-10-13 16:48:03', 34203: '2018-10-13 16:48:03', 34204: '2018-10-13 16:48:03', 34205: '2018-10-13 16:48:03', 34206: '2018-10-13 16:48:03', 34207: '2018-10-13 16:48:03', 34208: '2018-10-13 16:48:03'

Before moving my script to production, I need your suggestion for any improvement in my code.

Can anyone please have a look on my code and let me know any improvement in it?

Let me know also if I forgot to include any required information here.

1. The description of the problem in the post says that the case to be detected is redeeming "over 3 time" but what the code actually detects is 3 times or more. Which is right?



2. 
   

   The description of the problem in the post says that the case to be detected is redeeming:

   
   
   

   
   

   on same stamp card within 10 mins

   
   

   
   
   

   But what the code actually detects is redeeming:

   
   
   

   
   

   on same stamp card by the same user within 10 mins

   
   

   
   
   

   Which of these is right? It seems to me that the description in the post would be better, because otherwise someone could avoid detection by switching to a new user account after redeeming twice. (Maybe the software ensures that each stamp card is redeemable only by one user, but in that case why check the user at all?)

   
   


3. 
   

   The code assumes that the database will return records for each same stamp card id in increasing stamp time order, otherwise this won't work:

   
   
   

   g.tail(1).stamp_time.values - g.head(1).stamp_time.values
   
   

   
   
   

   But the SQL query only orders them by stamp card id, not by stamp time. Perhaps it happens to be the case at the moment that records are added to the table in stamp time order. But it is a bad idea to depend on this, because a change to the way the database is updated could break this assumption. It would be more robust to be explicit and write:

   
   
   

   ORDER BY stamp_card_id, stamp_time
   
   

   
   


4. 
   

   Here's what I suspect is the main performance problem:

   
   
   

   for i in range(0, len(unique_users)):
   
       user = unique_users[i]
   
       new_df = df[df.user_id == user]
   
   

   
   
   

   The expression df[df.user_id == user] looks innocent, but actually it has to loop over all the records in the dataframe twice: once to test user_id == user, and a second time to select the matching records. And this has to be done for every user. So if there are 36,000 records and (say) 10,000 unique users, then the user_id == user test gets run 360,000,000 times!

   
   
   

   Instead of iterating over the unique users and then finding the records for each user, use Panda's groupby method to efficiently group the records by user.

   
   
   

   But looking at my point §2 above, is this step really necessary? Perhaps what we really need to do is to ignore the users altogether and go straight to the stamp card ids.

   
   


5. 
   

   The same anti-pattern occurs in the stamp card processing code:

   
   
   

   sid = new_df.stamp_card_id.unique()
   
   for i in sid:
   
       fdf = new_df[new_df.stamp_card_id == i]
   
   

   
   
   

   This needs to become:

   
   
   

   for fdf in df.groupby(['stamp_card_id']):
   
   

   
   
   

   If you really do care about the users as well as the stamp cards, then you should order the query results by both:

   
   
   

   ORDER BY user_id, stamp_card_id, stamp_time
   
   

   
   
   

   and group by both:

   
   
   

   for fdf in df.groupby(['user_id', 'stamp_card_id']):
   
   

   
   


6. 
   

   The code creates a multiprocessing pool, but does not use it! You have to call the pool's methods, for example multiprocessing.Pool.apply, to take advantage of the pool—it doesn't work by magic.

   
   
   

   But actually I suspect that there's no need to use multiprocessing here. Once you have eliminated the quadratic runtime behaviour as discussed above, then it should run in an acceptable amount of time in a single process.