-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
created function for predicted unit value #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and functions look really good, nice work Giulia!
I've left a few comments which should be easy changes, once made it can be merged.
One comment is just something to be aware off and might not need changing right now
""" | ||
|
||
df = df.loc[(df["sampled"] == 1) & (df["nw_ag_flag"] == 0)] | ||
df = df.reset_index(drop=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, why do you need to reset and drop the index? I'm just thinking that other functions have set the index to be period
and date
columns and resetting might cause issues in the future. This might just be something to make a note of and see if it does lead to issues later :)
lambda x: x * (sum_weighted_target_values / sum_weighted_auxiliary_values) | ||
) | ||
|
||
return df |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function and docstrings look good
|
||
import pandas as pd | ||
|
||
df = pd.read_csv(Path("tests") / "data" / "winsorisation" / "predicted_unit_value.csv") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading this data is not needed in your main function script. I commented it out on mine and unit tests still passed.
You will need to delete it or break it over multiple commented lines to pass commit hooks
@@ -0,0 +1,58 @@ | |||
from pathlib import Path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove line 5 loading df, you need to remove this import to pass commit hooks
@pytest.fixture(scope="class") | ||
def predicted_unit_value_test_output(): | ||
return pd.read_csv( | ||
"tests/data/winsorisation/predicted_unit_value_output.csv", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could change this file path to be same as L13, or vice versa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giuliag92 Pushed changes caused by pre-commit hooks. Rest of the code looks good. Going to merge now :)
Summary
Created function to calculate the predicted unit value
Checklists
This pull request meets the following requirements:
If you feel some of these conditions do not apply for this pull request, please
add a comment to explain why.