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

Modifications to datasets.py #141

Merged
merged 9 commits into from
Feb 7, 2023
Merged

Conversation

kadri-nizam
Copy link
Contributor

  • Type hints added to datasets.py
  • Assertions converted to raised exceptions
  • Some __init__ functions' logic have been simplified by implementing guard clauses
  • KFoldCrossValidator now has a classmethod that intializes itself from Dartboard q_edges and phi_edges parameter
  • Updated and regenerated HD143006 documentation
    • Documentation now uses Gridder.from_image_properties initializer

Started type hinting Dartboard. The conditional statements can be simplified by rewriting them as guard clauses.

Copied attributes from coords can be rewritten as `@property` to save memory
The numpy type hints are a little long, but Python 3.8 doesn't support type aliasing.
"Beautiful is better than ugly."
The init logic can be simplified by treating the if statement as a guard clause
init function has been simplified as well
@kadri-nizam kadri-nizam mentioned this pull request Feb 6, 2023
2 tasks
@kadri-nizam kadri-nizam requested a review from iancze February 6, 2023 20:20
@@ -131,7 +131,7 @@ def __init__(self, cell_size: float, npix: int) -> None:
self.packed_q_centers_2D = np_fft.fftshift(self.sky_q_centers_2D)
self.packed_phi_centers_2D = np_fft.fftshift(self.sky_phi_centers_2D)

self.q_max = (
self.q_max = float(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just curious, what's the reason for the float call? Shouldn't this already be as a float type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the numpy fft calls end up returning values that the linter can't infer its type from. The float call just makes it explicit for use with mypy later.

@@ -126,7 +155,7 @@ def to(self, device):


# custom dataset loader
class UVDataset(Dataset):
class UVDataset(torch_ud.Dataset):
Copy link
Collaborator

@iancze iancze Feb 6, 2023

Choose a reason for hiding this comment

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

I think this class is stale and we'll want to eventually rewrite it, using some of the ideas put forth in #126. But for now it can come along for the ride. If I recall correctly, is it used anywhere else in the codebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right -- it is stale. I searched for references in the code base but there were none. I thought I'd keep it there in case I missed it.

Copy link
Collaborator

@iancze iancze left a comment

Choose a reason for hiding this comment

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

Great! Thanks for implementing these changes and spreading the type hints throughout the package. You can merge when ready.

@kadri-nizam kadri-nizam merged commit 38a50cd into MPoL-dev:main Feb 7, 2023
@kadri-nizam kadri-nizam deleted the datasets_file branch February 7, 2023 05:02
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.

2 participants