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

Refactor: Use NumpySample Type Alias in Function Return Type #181

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

Conversation

siddharth7113
Copy link
Contributor

Description

This PR replaces the return type annotation dict with NumpySample in the convert_gsp_to_numpy_sample function. This aligns the function signature with the existing type alias NumpySample, improving readability and maintainability.

= Changes Made

  • Updated convert_gsp_to_numpy_sample to return NumpySample instead of dict
  • Ensured consistency with type alias definitions

Fixes #177

How Has This Been Tested?

This change does not alter functionality but only refines type annotations.
The test suite was executed using pytest, and all tests passed successfully.

  • Ran pytest to ensure no regressions

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 (if applicable)
  • I have checked my code and corrected any misspellings

@Sukh-P
Copy link
Member

Sukh-P commented Feb 24, 2025

@siddharth7113 thanks for picking this up, we have made a few changes now, could you please try to import the NumpySample class again and see if the circular import errors are there? If so it would be good if we addressed those, also there are other files in the same folder of the folder you made this change to where it would be good to add the NumpySample return type

@siddharth7113
Copy link
Contributor Author

@siddharth7113 thanks for picking this up, we have made a few changes now, could you please try to import the NumpySample class again and see if the circular import errors are there? If so it would be good if we addressed those, also there are other files in the same folder of the folder you made this change to where it would be good to add the NumpySample return type

Sure, I will look into updated branch and check if things are working, also I think there were some issues with black for time being should I just skip that?

@siddharth7113 siddharth7113 reopened this Mar 3, 2025
@siddharth7113
Copy link
Contributor Author

Hi @Sukh-P ,

Sorry this took so long, I got sick in between, and hence couldn't update things, but the issue of circular import is still there.

@siddharth7113
Copy link
Contributor Author

Hi @Sukh-P , Requesting a review whenever you have time, this has no more conflitcs now, and tests are passing.

@@ -2,8 +2,10 @@

import numpy as np

from ocf_data_sampler.numpy_sample.base import NumpySample
Copy link
Member

@Sukh-P Sukh-P Mar 10, 2025

Choose a reason for hiding this comment

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

This should be NumpyBatch not NumpySample as the function stacks samples into a batch

@Sukh-P
Copy link
Member

Sukh-P commented Mar 10, 2025

@siddharth7113 thanks for the work on this and getting around the circular import error! However I think that moving the base file which was in the sample folder into the numpy sample folder might confuse things a bit, I think that these lines can stay in the numpy_sample directory, but in a file called common_types.py and the rest of the code can remain in a base file in the sample folder, I think there should not be any circular imports this way but if there is then do shout, thanks!

Copy link
Member

@Sukh-P Sukh-P left a comment

Choose a reason for hiding this comment

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

Left a bit of feedback on the folder structure, thanks!

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.

Use NumpySample Type Alias in numpy sample folder
2 participants