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.

Sign up for GitHub

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

Jump to bottom

fix: renamed numpy_batch to numpy_sample throughout the codebase #105

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix: renamed numpy_batch to numpy_sample throughout the codebase #105

wants to merge 9 commits into from

Conversation

Tim1119
Copy link

@Tim1119 Tim1119 commented Dec 23, 2024 •
edited
Loading

Pull Request

Description

This pull request renames the numpy_batch folder to numpy_sample because we only deal with samples, not batches. All relevant references and imports have been updated to reflect this change. The modification aligns the naming convention with the actual functionality for better clarity and consistency.

Fixes #103

How Has This Been Tested?

  • Ran unit tests to verify functionality after renaming the directory.

  • Checked all imports and ensured there were no unresolved references.

  • Verified that the application runs without errors after the changes.

  • Yes

The renaming does not change the logic of data processing, but it does ensure the terminology accurately reflects the operations being performed. I have verified that it runs successfully after the changes, ensuring there is no disruption in functionality.

  • 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.

@peterdudfield peterdudfield reopened this Dec 23, 2024
@peterdudfield
Copy link
Contributor

Thanks so much @Tim1119 for doing this and jumping on this so quickly

@Tim1119
Copy link
Author

Tim1119 commented Dec 24, 2024

Thanks so much @Tim1119 for doing this and jumping on this so quickly

You're welcome

@Tim1119
Copy link
Author

Tim1119 commented Dec 24, 2024

Thank you for the suggestions @felix-e-h-p . I'll implement the changes, refactoring the function into modular helpers as recommended while ensuring the core behavior and tests remain the same.

@Tim1119
Copy link
Author

Tim1119 commented Jan 1, 2025

Hi @felix-e-h-p and @peterdudfield, happy new year. I've implemented the suggested changes and resolved the issues. Let me know if there's anything else needed or if this is ready to be merged. Thanks

@peterdudfield
Copy link
Contributor

Thanks so much for this. I know @Sukh-P and @dfulu are using this code a lot, so I wont mind them checking, if thats ok

@Tim1119
Copy link
Author

Tim1119 commented Jan 2, 2025

Thanks so much for this. I know @Sukh-P and @dfulu are using this code a lot, so I wont mind them checking, if thats ok

Thank you, @peterdudfield . That sounds good. I'll wait for @Sukh-P and @dfulu to review it and will address any additional feedback if needed.

@dfulu dfulu self-requested a review January 2, 2025 19:55
dfulu
dfulu reviewed Jan 2, 2025
View reviewed changes
ocf_data_sampler/numpy_sample/collate.py Outdated
}

def _key_is_constant(sample_key):
is_constant = sample_key.endswith("t0_idx") or sample_key == NWPSampleKey.channel_names
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better style to just return this directly rather than to assign to a variable and then return

Copy link
Member

Choose a reason for hiding this comment

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

I just realised this assignment and return was in the original function. So perhaps don't worry about this in this PR unless you want to leave things cleaner than you found them :)

Copy link
Member

Choose a reason for hiding this comment

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

@Tim1119 thanks for making the the extra effort of addressing this one

dfulu
dfulu requested changes Jan 2, 2025
View reviewed changes
Copy link
Member

@dfulu dfulu left a comment

Choose a reason for hiding this comment

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

@Tim1119 thanks for all this work. There a just a couple of things which I think should be cleaned up, but besides that its all looking good!

ocf_data_sampler/numpy_sample/gsp.py Outdated Show resolved Hide resolved
ocf_data_sampler/numpy_sample/nwp.py Show resolved Hide resolved
ocf_data_sampler/numpy_sample/site.py Outdated Show resolved Hide resolved
tests/numpy_sample/test_gsp.py Outdated Show resolved Hide resolved
ocf_data_sampler/numpy_sample/collate.py Outdated
}

def _key_is_constant(sample_key):
is_constant = sample_key.endswith("t0_idx") or sample_key == NWPSampleKey.channel_names
Copy link
Member

Choose a reason for hiding this comment

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

I just realised this assignment and return was in the original function. So perhaps don't worry about this in this PR unless you want to leave things cleaner than you found them :)

ocf_data_sampler/numpy_sample/collate.py Show resolved Hide resolved
@dfulu
Copy link
Member

dfulu commented Jan 2, 2025

@peterdudfield when this is merged we must remember to put a issue into PVNet since the change from XBatchKey to XSampleKey will break compatibility

@Tim1119
Copy link
Author

Tim1119 commented Jan 3, 2025

Good day @peterdudfield ,

I have implemented the changes suggested by @dfulu .

Summary of Changes

  1. Return value directly in _key_is_constant function for improved readability
  2. Moved logic from add_osgb and create_data back to convert_nwp_to_numpy_sample function for simpicity and consistency
  3. Removed redundant None checks in convert_gsp_to_numpy_sample and convert_site_to_numpy_sample functions
  4. Removed GSPSampleKey.t0_idx in test_gsp.py since the call to convert_gsp_to_numpy_sample did not set the t0_idx parameter
  5. Moved stack_nested_keys(dict_list, parent_key, child_key) logic to process_nwp_data. Additionally I moved stack_data_for_key() to stack_np_examples_into_sample as suggested

Testing

I ensured that all tests ran successfully before and after making the changes.

Review Status:

Waiting for review from @Sukh-P

@dfulu
Copy link
Member

dfulu commented Jan 6, 2025

@Tim1119 thanks for all the additional work. I think its looking really good now. Could you have a look at the merge conflicts and then we should be ready to go 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felix-e-h-p felix-e-h-p felix-e-h-p left review comments

@dfulu dfulu dfulu requested changes

@peterdudfield peterdudfield peterdudfield approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change numpy_batch to numpy_sample
4 participants
@Tim1119 @peterdudfield @dfulu @felix-e-h-p