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

fix: remove key_prefix from time features #162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anxkhn
Copy link

@anxkhn anxkhn commented Feb 9, 2025

Pull Request

Description

This pull request removes the key_prefix argument from the make_datetime_numpy_dict and make_sun_position_numpy_sample functions and updates all call sites and tests accordingly. The key_prefix was originally introduced for flexibility in naming output dictionary keys, primarily within the context of the now-deprecated ocf_datapipes library. Since ocf-data-sampler uses a different approach (PyTorch Datasets instead of DataPipes), this flexibility is no longer needed and introduces unnecessary complexity.

This PR simplifies the code by hardcoding consistent key names for the datetime and sun position features:

  • make_datetime_numpy_dict: Keys are now "date_sin", "date_cos", "time_sin", "time_cos".
  • make_sun_position_numpy_sample: Keys are now "solar_azimuth", "solar_elevation".

This change improves code readability and maintainability by removing a parameter that is no longer relevant and ensuring consistent naming across the codebase.

Fixes #159

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 (No docstring changes were necessary, as the functions' behavior didn't fundamentally change, only the output keys)
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@Omnikam00007 Omnikam00007 mentioned this pull request Feb 9, 2025
5 tasks
@Sukh-P
Copy link
Member

Sukh-P commented Feb 12, 2025

Thanks for making a start on this @anxkhn, I will assign you to the issue so there is no confusion on who is working on this, I think some of the tests still need updating as the expect the key prefix still and there are some merge conflicts but hopefully should be good to go after that!

@anxkhn
Copy link
Author

anxkhn commented Feb 13, 2025

Hi, I will work on resolving tests soon.

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.

remove key_prefix from time features
2 participants