Ability to specify additional locations of DataFrame to check - follow up

[drosengarten]

Hi,
This is a follow up from:
#57

The point of the issue is for a pass through function. It is a small bit of code, but can clearly make the parameters needed for a function specified. It is easier for both a human and IDEs to be able to determine that the function

@ck.has_columns(df='df',columns=['grade','extra_credit','total_assignment_score'])
def aggregate_totals(df):

needs three columns with specific names. the user of the code shouldn't have to read through the source to determine the names, and just putting into docstring doesn't enforce the definition. Self documenting code is better than a docstring.

Also, this issue also covers multiple return dataframes that the current implementation does not implement.

I have the code (it is short and sweet) and and the associated tests that I can create a pull request from.

Thanks

[ZaxR]

Can you show a before and after for the above example, so I'm clear on this? Also, will this be a breaking change?

[drosengarten]

Hi,
This isn't a breaking change. The 2 intended purposes are:

1. To allow checking on all the in/out paramaters, including tuples
2. To allow better documentation/checking on the function signatures.

Not a breaking change:
In the associated pull request, all the prior tests have been unchanged. the additional tests show the capability.
Using the extra decorator parameter df:

import bulwark.checks as ck
import bulwark.decorators as dc

@dc.has_columns(columns=['Item','Total'])
@dc.has_columns(df=None,columns=['Item','Total'])

Are equivalent (and most people wont use the latter form).

This allows the user to have checks on returned tuples like:

from typing import Tuple
@dc.has_columns(df=0,columns=['Item','Total'])
@dc.has_columns(df=1,columns=['Name','Address'])
def test_tuple()->Tuple[pd.DataFrame, pd.DataFrame]:
    df_items = pd.DataFrame(.......)
    df_clients = pd.DataFrame(.......)
    return df_items, df_clients

The example above couldn't have been performed without the change unless two other functions were created or by using the checks directly in code like the following:

def test_tuple()->Tuple[pd.DataFrame, pd.DataFrame]:
   df_items = pd.DataFrame(.......)
   df_clients = pd.DataFrame(.......)
   ck.has_columns(df=df_items,columns=['Item','Total'])
   ck.has_columns(df=df_clients,columns=['Name','Address'])
   return df_items, df_clients

The above doesn't inform the caller what is being returned, so the caller will need to read the source, which wouldn't exist for a method in an abstract base class

This extra parameter can help clarifies what is needed by the function if it is used for inputs:

@dc.has_columns(df='prices',columns=['item','total'])
@dc.has_columns(df='transactions',columns=['clientId','item','amount'])
@dc.has_columns(columns=['client_id','total'])
def calculate_total(prices:pd.DataFrame, df_transactions:->pd.DataFrame
   ....
   return df_total

Again, you can embed the checks somewhere in the method, but that doesn't help the caller without digging through the source.

[ZaxR]

Do you have a real world example of when you'd actually want a function to accept or output a tuple of dataframes?

Generally it's considered bad practice to output a variable number of items from a function, and even worse to have functions that can return multiple types (tuple or dataframe). While allowed it Python, it can increase bugs and make them much harder to debug.

In the example test_tuple above, you have a function that takes no inputs, which is pretty uncommon except as a pipeline wrapper procedure. For the sake of the demo it makes sense for data creation, but otherwise I'd always expect a parameter to a function. I'm also not sure why you'd write a function to load data or make a pipeline that has multiple outputs. If you're in that situation, it's usually a code smell indicating it's time to break down the function into multiple smaller functions.

I'm also a little put off my the df=0 or df='prices' syntax. If I'm reading that right, df=0 and df='prices' (despite looking like df is the int 0 or a string 'prices' ) are actually the index in the return tuple or the input df type of data? To me that's pretty confusing without reading some docs at the very least.

Lastly, calculate_total, without seeing the code masked by ..., seems like it's a function that somehow combines two dfs (prices and transactions) and then aggregates them. I'd argue rather than specifying the property tests for two dfs, what you really want is to split this function into two functions, each with it's own purpose:

1. combine the two dfs
2. calculate_total

In that case, I would suggest the previous functions that create each of the prices and transactions dfs have their own respective has_columns checks, and that the function that combines the two dfs would have a decorator to check the columns on the combined df.

Example of how I'd write this whole pipeline, including current Bulwark syntax for checks:

import bulwark.decorators as dc
import pandas as pd


@dc.HasColumns(columns=['item', 'total'])
def create_price_df(): -> pd.DataFrame
    df = pd.DataFrame({'item': ["a", "b", "c"], 'total': [1, 2, 3]})
    return df

@dc.HasColumns(columns=['client_id', 'item', 'amount'])
def create_transactions_df() -> pd.DataFrame:
    df = pd.DataFrame({'client_id': [567, 189, 488], 'item': ["a", "b", "c"], 'amount': [1, 2, 3]})
    return df

@dc.HasColumns(columns=['client_id', 'item', 'amount', 'total'])
def merge_price_trans_dfs(price_df, transactions_df) -> pd.DataFrame:
    merged_df = transactions_df.merge(price_df, how='left', on='item')
    merged_df = merged_df[['client_id', 'item_x', 'amount', 'total']].rename(columns={'item_x': 'item'})
    return merged_df

# not clear on what check you want here
# My best guess would be the func is meant to group by client_id?
def calculate_total(merged_df) -> pd.DataFrame:
    ....
    return df_total

price_df = create_price_df()
transactions_df = create_transactions_df()
merged_df = merge_price_trans_dfs(price_df, transactions_df)
df_total = calculate_total(merged_df)

Please let me know if I'm misunderstanding the purpose of any of the sample functions.

[drosengarten]

Hi,
There are a few libraries (including internal libraries), which require an output / input of more than one dataframe. I agree that it is not the best coding style, but I can't change the other libraries. I would rather have some checks on the input/output than none at all.
I wrote the examples above to be very general and made up to demonstrate the use, not a specific use-case. My use cases are very specific and different, but do require multiple dataframe outputs. I do like the ability to add information about the dataframe above the function definition in order to eliminate a caller to dig through the code to find out some basic info (column names) of the dataframe. This use can be eliminated, but I think a general way to specify any input/output parameter would be welcome.

If you don't like the df=0 or df='prices', we can add two other decorators check_output_tuple and check_input.

The usefulness of the library is the ability to put the checks a dataframe in code/decorators. The code portion is fine (I have some other suggestions about enable/disable hooks), but the decorators are currently not flexible enough to be used in all the real-world cases.

[drosengarten]

@ZaxR , Will you please consider either the current implementation or two other monikers for the two other situations that the pandas dataframes can be checked? Thanks.

[ZaxR]

Hey - thanks for following up. I didn't forget about you, just been busy on a couple other open source projects. I wanted to take some time to test-drive the version from your PR so I could consider alternative interfaces, if the implementation looks like what you have. Hopefully I'll get to do that tomorrow, but this weekend might have to prioritize pandas 1.0-related maintenance.

In the interest of making my test driving easier, can you share the libraries you mentioned that have the tuple output? And any open source code examples you might have? Will save me time developing some to test out.

[mlisovyi]

I wanted just to add +1 to the proposal. For an internal project we are evaluating various data validation tools for pandas dataframes. Our code is not perfect, so we have plenty of code that has more than 1 DF as input and more than 1 DF out. To my understanding this proposal does not suggest to support variable output length, but rather to have specific validators for specific elements of a fixed-length tuple of dataframes (@drosengarten please correct me if i got it wrong).

So current implementation makes decorators not particularly useful for us, neglecting the style advantages of those.

Wouldn't sklearn.model_selection.train_test_split or pd.merge be examples of such functions that have not a 1-in-1-out structure?