Skip to content

km-kill-lags branched off frosting. #77

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

Merged
merged 31 commits into from
Jul 8, 2022
Merged

km-kill-lags branched off frosting. #77

merged 31 commits into from
Jul 8, 2022

Conversation

kenmawer
Copy link
Contributor

Closes issue #22.

@kenmawer kenmawer requested a review from dajmcdon as a code owner June 28, 2022 21:12
@dajmcdon
Copy link
Contributor

@kenmawer I'm not sure what the error is here, but this needs some debugging.

@kenmawer
Copy link
Contributor Author

kenmawer commented Jun 29, 2022

@dajmcdon These errors seem to be coming from tests of functions such as those relating to frosting, none of which I worked on. Here's an example from test-frosting:
Error in validate_is_function_set(mold): argument "mold" is missing, with no default

@dajmcdon
Copy link
Contributor

Right, but those errors come from tests that use step_epi_lag(). So something that this branch does broke those tests. Two things I saw:

  • Calling get_test_data() no longer works.
  • In test-frosting.R on line 46, the predictions have the wrong date.

@kenmawer kenmawer closed this Jun 30, 2022
@kenmawer kenmawer reopened this Jun 30, 2022
Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

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

Note that you can safely edit this branch locally and push to the same remote.

@dajmcdon
Copy link
Contributor

dajmcdon commented Jul 7, 2022

If you merge frosting onto this branch, then the checks should pass.

@kenmawer kenmawer requested a review from dajmcdon July 8, 2022 15:52
@kenmawer
Copy link
Contributor Author

kenmawer commented Jul 8, 2022

As described earlier, splitting the print function means that the print won't work properly for lags.

@dajmcdon
Copy link
Contributor

dajmcdon commented Jul 8, 2022

@kenmawer I fixed up the printing to work the way we discussed in my office. Why don't you pull locally, run the examples, and see what you think. Have a look at how I did it just to make sure that it makes sense. Once it does, you can approve this PR and merge merge this PR (I approve).

@kenmawer kenmawer merged commit 665b86a into frosting Jul 8, 2022
@dajmcdon dajmcdon deleted the km-kill-lags-good branch July 8, 2022 21:06
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