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

Move copy_batch_to_device to this repo #168

Closed
Sukh-P opened this issue Feb 14, 2025 · 4 comments
Closed

Move copy_batch_to_device to this repo #168

Sukh-P opened this issue Feb 14, 2025 · 4 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@Sukh-P
Copy link
Member

Sukh-P commented Feb 14, 2025

We want to remove any dependencies the PVNet repo has with ocf_datapipes as we plan to deprecate that repo so it would be good to move this function https://github.com/openclimatefix/ocf_datapipes/blob/main/ocf_datapipes/batch/utils.py#L9 to this repo, can probably live here https://github.com/openclimatefix/ocf-data-sampler/blob/main/ocf_data_sampler/sample/base.py

@alirashidAR
Copy link
Contributor

I would like to take this up @Sukh-P !

@Sukh-P
Copy link
Member Author

Sukh-P commented Feb 14, 2025

Great, thank you @alirashidAR ! You might be interested in doing the linked issue openclimatefix/PVNet#320 after this too

@alirashidAR
Copy link
Contributor

@Sukh-P I wasn’t aware that a PR had already been made, so I worked on the functionality myself. Since I was originally assigned to this issue, I’d be happy to review @zaryab-ali’s PR first. Once it’s merged, I’ll proceed with the next issue on my own.

@Sukh-P
Copy link
Member Author

Sukh-P commented Feb 18, 2025

@alirashidAR since you were assigned to this issue first I would say that the PR you have created can be reviewed first, I have done that now and I am gong to merge it in.

Apologies @zaryab-ali we are updating our contribution guidelines to make this clearer but we are asking people to comment on issues first that they would like to pick up that work, then get assigned, then create the PRs, this is to avoid duplication of work and to make sure the issue being picked up we feel is suitable for the OS community

@Sukh-P Sukh-P closed this as completed Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants