Skip to content
This repository has been archived by the owner on Sep 11, 2023. It is now read-only.

Rename time_resolution_minutes to sample_period_minutes to make the code consistent #587

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

Rename time_resolution_minutes to sample_period_minutes to make the code consistent #587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8bf21df
changed-sample_period_minutes
RishiKumarRay Jan 27, 2022
c74ba90
Merge branch 'main' into fix-sample-period-min
JackKelly Jan 27, 2022
b51abb4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jan 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions nowcasting_dataset/config/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,14 +126,13 @@ def history_seq_length_60_minutes(self):
class TimeResolutionMixin(Base):
"""Time resolution mix in"""

# TODO: Issue #584: Rename to `sample_period_minutes`
time_resolution_minutes: int = Field(
sample_period_minutes: int = Field(
5,
description="The temporal resolution (in minutes) of the satellite images."
"Note that this needs to be divisible by 5.",
)

@validator("time_resolution_minutes")
@validator("sample_period_minutes")
def forecast_minutes_divide_by_5(cls, v):
"""Validate 'forecast_minutes'"""
assert v % 5 == 0, f"The time resolution ({v}) is not divisible by 5"
Expand Down
6 changes: 3 additions & 3 deletions nowcasting_dataset/data_sources/optical_flow/optical_flow_data_source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class OpticalFlowDataSource(DataSource):
the input image after it has been "flowed".
source_data_source_class_name: Either HRVSatelliteDataSource or SatelliteDataSource.
channels: The satellite channels to compute optical flow for.
time_resolution_minutes: time resolution of optical flow images.
sample_period_minutes: time resolution of optical flow images.
Needs to be the same as the satellite images used to make the flow fields.
"""

Expand All @@ -65,7 +65,7 @@ class OpticalFlowDataSource(DataSource):
meters_per_pixel: int = 2000
output_image_size_pixels: int = 32
source_data_source_class_name: str = "SatelliteDataSource"
time_resolution_minutes: int = 5
sample_period_minutes: int = 5

def __post_init__(self): # noqa
assert self.output_image_size_pixels <= self.input_image_size_pixels, (
Expand Down Expand Up @@ -98,7 +98,7 @@ def __post_init__(self): # noqa
@property
def sample_period_minutes(self) -> int:
"""Override the default sample minutes"""
return self.time_resolution_minutes
return self.sample_period_minutes
Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I hadn't thought about this issue when I wrote issue #587! I don't think Python allows classes where a @property method has the same name as an attribute of the class. (Because, in Python, methods are attributes, too 🙂). Sorry, I should've thought about this!

Perhaps the cleanest solution is to completely get rid of the sample_period_minutes @property method from DataSource and all subclasses of DataSource. But then I think we bump into the issue that dataclass subclasses can't set new default values. (Although maybe that's changed in more recent versions of Python?)

If you're up for it, it'd be amazing if you could have a tinker and see if it's possible to remove the sample_period_minutes @property method, and then find a way to allow subclasses of DataSource to set their own default values for sample_period_minutes?

Copy link
Author

Choose a reason for hiding this comment

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

@JackKelly I have to learn python for this, I don't know much about python

Copy link
Member

Choose a reason for hiding this comment

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

OK, no rush! It's totally up to you: If you fancy doing this then please go for it, there's no rush for this issue! But, if you'd prefer not to, then please just shout!

The relevant part of the Python docs might be this page on inheritance in dataclasses. On skim-reading the docs, it actually looks like it might be fine to just delete the sample_period_minutes @property method from all our code, and just use a sample_period_minutes attribute, which can be set to different values for each subclass.

Copy link
Author

Choose a reason for hiding this comment

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

@JackKelly Sorry for this , I thought it was all about renaming thats why I raised the PR , maybe I am not suitable for this now to do the changes , If you want i will close this pull request and maybe you can assign someone else, i looked at the problem , removing sample_period_minutes @Property method is easy as you know but using just as atribute , I don't know much of it since i am totally unaware of python.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, absolutely no worries! Don't worry about deleting this PR, as your changes are definitely useful! Maybe someone else can pick up where this left off... Thanks again for your help, and please do keep an eye out for other "renaming" issues!

Copy link
Author

Choose a reason for hiding this comment

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

@JackKelly If it is useful , can you merge it , maybe some other issue should be opened for adding as attribute , Just a suggestion , Thanks :) also I will start learning python in my free time , thanks for suggestions

Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain but I think the code might break if we merge as is (even though the unittests pass... but I think the passing tests might be a "false pass" because maybe we don't have any unittests which call the sample_period_minutes @property?)

Copy link
Author

Choose a reason for hiding this comment

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

@JackKelly yeah got your point , lets wait then for someone to take this


def open(self):
"""Open the underlying self.source_data_source."""
Expand Down
4 changes: 2 additions & 2 deletions nowcasting_dataset/data_sources/satellite/satellite_data_source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class SatelliteDataSource(ZarrDataSource):
image_size_pixels: InitVar[int] = 128
meters_per_pixel: InitVar[int] = 2_000
logger = _LOG
time_resolution_minutes: int = 5
sample_period_minutes: int = 5

def __post_init__(self, image_size_pixels: int, meters_per_pixel: int):
"""Post Init"""
Expand All @@ -52,7 +52,7 @@ def __post_init__(self, image_size_pixels: int, meters_per_pixel: int):
@property
def sample_period_minutes(self) -> int:
"""Override the default sample minutes"""
return self.time_resolution_minutes
return self.sample_period_minutes

def open(self) -> None:
"""
Expand Down
2 changes: 1 addition & 1 deletion tests/config/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def test_incorrect_time_resolution():

configuration = Configuration()
configuration.input_data = configuration.input_data.set_all_to_defaults()
configuration.input_data.satellite.time_resolution_minutes = 27
configuration.input_data.satellite.sample_period_minutes = 27
with pytest.raises(Exception):
_ = Configuration(**configuration.dict())

Expand Down
4 changes: 2 additions & 2 deletions tests/data_sources/satellite/test_satellite_data_source.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def test_geospatial_border(sat_data_source): # noqa: D103


def test_wrong_sample_period(sat_filename):
"""Test that a error is raise when the time_resolution_minutes is not divisible by 5"""
"""Test that a error is raise when the sample_period_minutes is not divisible by 5"""
with pytest.raises(Exception):
_ = SatelliteDataSource(
image_size_pixels=pytest.IMAGE_SIZE_PIXELS,
Expand All @@ -154,5 +154,5 @@ def test_wrong_sample_period(sat_filename):
forecast_minutes=15,
channels=("IR_016",),
meters_per_pixel=6000,
time_resolution_minutes=27,
sample_period_minutes=27,
)