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

Conversation

@jacobbieker
Copy link
Contributor

@jacobbieker jacobbieker commented Nov 3, 2021

Pull Request

Description

Removes the Datetime Data Source, as that isn't used anymore and the features are now computed on the fly in the dataloader.

Fixes #208

How Has This Been Tested?

Unit tests

  • No
  • Yes

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

@jacobbieker jacobbieker added enhancement New feature or request refactoring labels Nov 3, 2021
@jacobbieker jacobbieker self-assigned this Nov 3, 2021
@JackKelly
Copy link
Contributor

Nice!

I think there are also some other functions which compute datetime features and can be removed (along with any tests):

  • utils.sin_and_cos
  • time.datetime_features
  • time.datetime_features_in_example

@jacobbieker jacobbieker marked this pull request as ready for review November 3, 2021 12:24
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, Thanks for doing this

Is there any readme's to update?

Would it possible to get a one liek this ready for dataloader at the same time, then we can merge at the same point, and hopefully not have any problems

@jacobbieker
Copy link
Contributor Author

Looks good, Thanks for doing this

Is there any readme's to update?

Would it possible to get a one liek this ready for dataloader at the same time, then we can merge at the same point, and hopefully not have any problems

Yeah! I'll start on that

@jacobbieker
Copy link
Contributor Author

Its here: openclimatefix-archives/nowcasting_dataloader#47

@jacobbieker jacobbieker merged commit 9e8fb4c into main Nov 3, 2021
@jacobbieker jacobbieker deleted the jacob/delete-datetime branch November 3, 2021 13:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compute datetime features on-the-fly in nowcasting_dataloader. Remove datetime features from the on-disk batches.

4 participants