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

Allow for NA values in optional horizon time difference checks (#140) #146

Merged
merged 10 commits into from
Oct 31, 2024

Conversation

annakrystalli
Copy link
Member

Should be reviewed after #144 & #145 are merged

This PR allows for NA values in optional horizon time difference checks by subsetting tbl to complete cases of relevant columns. This ensures rows that may not be targeting relevant to modeling task do not cause false check failure.

Resolves #140

@annakrystalli annakrystalli requested a review from elray1 October 30, 2024 11:07
Copy link

github-actions bot commented Oct 30, 2024

Copy link
Contributor

@elray1 elray1 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

LGTM!

TIL about stats::complete.cases()!

As an aside, is there a reason why the content from #144 and #145 are in here?

@annakrystalli annakrystalli merged commit a75e337 into main Oct 31, 2024
10 checks passed
@annakrystalli annakrystalli deleted the ak/timediffs-ignore-NAs/140 branch October 31, 2024 07:29
@annakrystalli
Copy link
Member Author

As an aside, is there a reason why the content from #144 and #145 are in here?

Not sure. The diffs should have updated automatically when the other PR's were merged in. That's how it always worked before. GitHub has been a bit strange recently, like when it closed the PR when the branch it was pointing to was deleted. I'm, 100% sure in the past it just redirected the PR to main in such a situation.

@zkamvar
Copy link
Member

zkamvar commented Oct 31, 2024

Not sure. The diffs should have updated automatically when the other PR's were merged in. That's how it always worked before. GitHub has been a bit strange recently, like when it closed the PR when the branch it was pointing to was deleted. I'm, 100% sure in the past it just redirected the PR to main in such a situation.

But was this pattern you were observing based on the before R-universe workflow? In that workflow, there were always a chain of pull requests main <- A <- B <- C and they would be merged in the order of C, B, and finally A. When GitHub deleted the branch last time, it was because the same chain existed, but they were merged in the order of A, B, and finally C. When you merged A, the branch that B was pointing to no longer existed, so the pull request was closed.

In this case, you had A, B, and C all pointing to the main branch, but A, B, and C were still dependent on each other (because they were created sequentially).

I would recommend creating each new branch from main in the future.

@annakrystalli
Copy link
Member Author

annakrystalli commented Oct 31, 2024

Sometimes I need the features in the other PRs too.

It has worked in the past and when I need to I will still use this approach

@annakrystalli
Copy link
Member Author

annakrystalli commented Oct 31, 2024

Actually, thinking about it, it does make sense why it didn't work as I expected. As mentioned above I do often need to build on previous PRs. But what seems to be actually needed in this situation is a merge in between: isaacs/github#750 (comment) and three-dot-and-two-dot-git-diff-comparisons

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

In optional validation for horizon time difference, allow for NA values
3 participants