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

330 consecutive imputation links #15

Merged
merged 5 commits into from
May 21, 2024
Merged

Conversation

robertswh
Copy link
Collaborator

@robertswh robertswh commented May 16, 2024

Summary

Multiple consecutive missing values need imputation links to be multiplied to apply the correct imputation link. This function creates two new columns onto the input DataFrame, one for imputation_group that defines consecutive missing values, and another column for cumulative links.

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 self-requested a review May 17, 2024 12:20
Copy link
Collaborator

@Jday7879 Jday7879 left a comment

Choose a reason for hiding this comment

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

Nicely written code and unit test. Tests pass locally. Two minor comments which don't necessarily need to be changed

100,100000,,202404,3,1,2,6,1
200,100001,,202402,1,4,3,1,2
200,100001,,202403,3,0.5,3,3,0.5
200,100001,300,202404,0.5,1,4,0.5,1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might be misunderstanding the code / method, but why isn't cumulative_forward_imputation_link for the last row 1.5. Is that because it is in a different imputation group?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes that is correct. We only want to multiply the links where there are consecutive missing values. In fact, maybe we should turn all cumulative links in the same row as a return to nan because it has no meaning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to confirm, the last row is just the same as the imputation_link column because it is a return not a missing value. I have made a change to turn the cumulative link columns to NaN if there is a return

src/cumulative_imputation_links.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Jday7879 Jday7879 left a comment

Choose a reason for hiding this comment

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

Happy for the code to be merged following the minor changes

100,100000,,202404,3,1,2,6,1
200,100001,,202402,1,4,3,1,2
200,100001,,202403,3,0.5,3,3,0.5
200,100001,300,202404,0.5,1,4,,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this makes more sense now that the cumulative link doesn't return anything in cases where there is a valid return :)

@robertswh robertswh merged commit 0e4b261 into main May 21, 2024
3 checks passed
@robertswh robertswh deleted the 330-consecutive-imputation-links branch May 21, 2024 15:31
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.

2 participants