Fix for #108 - Improvements to data sampling logic #129
+100
−79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Updated the broken link to reference the correct torch datasets documentation
Co-authored-by: Sukhil Patel <[email protected]>
The key changes that are made in the code are - Column Renaming: Used DataFrame.rename() for consistent column renaming. Broadcasting Metadata: Simplified the creation of capacity_kwp using set_index() and loc[] to filter metadata efficiently. Validation: Added checks for required metadata columns to ensure robustness. Clarity: Reorganized and documented steps for better maintainability. The code is more efficient, easier to read, and robust against common errors.
Sukh-P
reviewed
Jan 30, 2025
README.md
Outdated
| @@ -18,7 +18,8 @@ We are currently migrating to this repo from [ocf_datapipes](https://github.com/ | |||
| ## Documentation | |||
|
|
|||
| **ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the Readme [here](https://github.com/openclimatefix/ocf-data-sampler/tree/readme-update/ocf_data_sampler/torch_datasets). | |||
| **ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the Readme [here]([torch datasets documentation](ocf_data_sampler/torch_datasets/README.md) | |||
There was a problem hiding this comment.
Suggested change
| **ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the Readme [here]([torch datasets documentation](ocf_data_sampler/torch_datasets/README.md) | |
| **ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the Readme [here](ocf_data_sampler/torch_datasets/README.md) |
Sukh-P
reviewed
Jan 30, 2025
|
Thanks for this @SophiaLi20, left a few small comments it would be good to address and then sort out the merge conflict, after that we can get this merged in, thanks! |
Added comments to test_config.py for better clarity Added inline comments to explain what each test case is checking. This should make it easier for others to understand the purpose of each test without having to go through the code in detail. No actual functionality was changed—just improving readability.
Improve Readability of test_config.py by Adding Inline Comments
Added inline comments to explain what each test case is checking. This should make it easier for others to understand the purpose of each test without having to go through the code in detail. No actual functionality was changed
SophiaLi20
commented
Feb 26, 2025
|
Hey @Sukh-P , I sincerely apologize for the delay—I had exams and couldn’t get back to this sooner. I've now implemented the suggested changes and fixed the merge conflict. Please have a look when you get a chance, and let me know if there's anything else I should change. I’ll make any necessary changes right away. Thanks for your patience and guidance! |
|
@SophiaLi20 sorry about the delay but is it okay to revert any changes you have made to the test_config file for now? I want to keep this PR to the original scope that you set out in the issue, then we can get this merged in, thanks! |
Sukh-P
requested changes
Mar 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the issue described in #108 and incorporates the feedback mentioned in this comment.
Key Changes:
Testing:
Additional Notes:
Checklist:
[ x] My code follows OCF's coding style guidelines
[x ] I have performed a self-review of my own code
[x ] I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
[x ] I have checked my code and corrected any misspellings