Skip to content
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

353 create imputation markers #14

Merged
merged 20 commits into from
May 22, 2024
Merged

Conversation

Jday7879
Copy link
Collaborator

@Jday7879 Jday7879 commented May 16, 2024

Summary

Added function which creates a logical column checking if the target variable is a return, or can be imputed by forward or backwards imputation or imputed by construction
Updated original unit tests ready to expand to create a single test dataset, but this should be added to the backlog and looked at further down the line

Checklists

This pull request meets the following requirements:

  • installable with all dependencies recorded
  • runs without error
  • follows PEP8 and project specific conventions
  • appropriate use of comments, for example no descriptive comments
  • functions documented using Numpy style docstings
  • assumptions and decisions log considered and updated if appropriate
  • unit tests have been updated to cover essential functionality for a reasonable range of inputs and conditions
  • other forms of testing such as end-to-end and user-interface testing have been considered and updated as required
  • tests suite passes (locally as a minimum)
  • peer reviewed with review recorded

If you feel some of these conditions do not apply for this pull request, please
add a comment to explain why.

@Jday7879 Jday7879 requested a review from AntonZogk May 16, 2024 10:26
@Jday7879 Jday7879 marked this pull request as ready for review May 16, 2024 10:26
@Jday7879 Jday7879 closed this May 16, 2024
@Jday7879 Jday7879 reopened this May 17, 2024
@Jday7879 Jday7879 marked this pull request as draft May 17, 2024 08:24
@Jday7879 Jday7879 marked this pull request as ready for review May 17, 2024 12:15
src/imputation_flags.py Outdated Show resolved Hide resolved
src/imputation_flags.py Outdated Show resolved Hide resolved
src/imputation_flags.py Outdated Show resolved Hide resolved
src/imputation_flags.py Outdated Show resolved Hide resolved
src/imputation_flags.py Outdated Show resolved Hide resolved
src/imputation_flags.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@AntonZogk AntonZogk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jday7879 lovely work for a complicated task love what you did with ffil and bffill smart solution :)

I have left some comments, happy to chat in more detail about them

AntonZogk added 3 commits May 22, 2024 12:24
Instead of calling flag_matched_pair_merge within the function to create the predictive_auxiliary, it is defined as function argument. Hence flag_matched_pair_merge must be called before create_impute_flags.  This will convert flag_matched_pair_merge  to a low level function and using pandas framework.
@AntonZogk
Copy link
Collaborator

I updated the function .

Instead of calling flag_matched_pair_merge within the function to create the predictive_auxiliary, it is defined as function argument. Hence flag_matched_pair_merge must be called before create_impute_flags. This will convert flag_matched_pair_merge to a low level function and using only pandas framework.

I added the predictive_auxiliary to test_data csv file and I also updated the tests accordingly to match expected columns

@AntonZogk AntonZogk requested a review from robertswh May 22, 2024 12:35
AntonZogk
AntonZogk previously approved these changes May 22, 2024
@AntonZogk AntonZogk dismissed their stale review May 22, 2024 12:38

Ticket needs new review from different developer

Copy link
Collaborator

@robertswh robertswh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, tests pass.

@robertswh robertswh merged commit a27bb91 into main May 22, 2024
3 checks passed
@robertswh robertswh deleted the 353-create-imputation-markers branch May 22, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants