Skip to content
This repository was archived by the owner on Sep 11, 2023. It is now read-only.

Set n_validation_batches to 0 #445

Merged
merged 8 commits into from
Nov 18, 2021
Merged

Set n_validation_batches to 0 #445

merged 8 commits into from
Nov 18, 2021

Conversation

@JackKelly
Copy link
Contributor

@JackKelly JackKelly commented Nov 18, 2021 •
edited
Loading

Pull Request

Description

Related to #426
Fixes #441

Still TODO: Merge in Peter's unit tests for this!

How Has This Been Tested?

  • No
  • Yes, unit tests pass
  • prepare_ml_data.py runs

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@JackKelly JackKelly self-assigned this Nov 18, 2021
@JackKelly JackKelly added the bug Something isn't working label Nov 18, 2021
@JackKelly
Copy link
Contributor Author

JackKelly commented Nov 18, 2021 •
edited
Loading

Still TODO: Merge in Peter's unit tests for this! Actually, sorry, the unit tests aren't directly applicable to this implementation!

@JackKelly JackKelly linked an issue Nov 18, 2021 that may be closed by this pull request
Split_data config #426
Open
@JackKelly
Copy link
Contributor Author

JackKelly commented Nov 18, 2021

Hi @peterdudfield .... so... this is maybe a bit awkward 🙂

This PR fixes #441 and maybe slightly addresses #426. I think this PR fixes the root issue, and also addresses a PR comment you had a while ago about adding descriptions to the n_<split_name>_batches fields in model.py!

What do you think about merging this PR into main?

peterdudfield
peterdudfield approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@peterdudfield peterdudfield 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,

I think a test case should be written to make sure this done error - either add it and show an error is made, or add an issue?

peterdudfield
peterdudfield reviewed Nov 18, 2021
View reviewed changes
nowcasting_dataset/manager.py
if (n_batches_requested == 0 and len(datetimes_for_split) != 0) or (
len(datetimes_for_split) == 0 and n_batches_requested != 0
):
msg = (
Copy link
Contributor

@peterdudfield peterdudfield Nov 18, 2021

Choose a reason for hiding this comment

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

good clear message

I ahve see {variable=} and thanks prints out variable={variable} as a short hand

Copy link
Contributor Author

@JackKelly JackKelly Nov 18, 2021

Choose a reason for hiding this comment

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

ooooh! f"{variable=}" is a really nice trick! Thanks loads for telling me about that! I'm gonna use that all over the place now 🙂

@JackKelly JackKelly mentioned this pull request Nov 18, 2021
Open
@JackKelly
Copy link
Contributor Author

JackKelly commented Nov 18, 2021

Good idea about adding a test! I've added that as issue #450, as I'm keen to get this merged into main. Thanks loads!

@JackKelly JackKelly merged commit 777e6f7 into main Nov 18, 2021
@JackKelly JackKelly deleted the bug/empty-t0-datetimes branch November 18, 2021 11:57
@JackKelly JackKelly mentioned this pull request Nov 18, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@peterdudfield peterdudfield peterdudfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JackKelly JackKelly

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Split_data config O validation items

3 participants

@JackKelly @peterdudfield