-
Notifications
You must be signed in to change notification settings - Fork 7
Issue/52 sun azimuth elevation #175
Conversation
…-sun-azimuth-elevation
…-sun-azimuth-elevation
assert len(azimuth) == len(datestamps) | ||
assert len(azimuth.columns) == N | ||
|
||
# 49 * 100 = 4,900 takes ~1 seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been moved not delete
) | ||
assert len(batch) == 5 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is no longer needed, as azimuth is not calculated in pv data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
I'm only half way through, but I've gotta go out for a few hours, so I wanted to send this review now! I'll pick it up again at sun_data_source.py ASAP...
Co-authored-by: Jack Kelly <[email protected]>
…tefix/nowcasting_dataset into issue/52-sun-azimuth-elevation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK! I've finished reviewing now! All looks great! Thank you for doing this!
One quick question (which I should really know the answer to!):
For a given time, do the Sun's angle and/or azimuth differ much from one side of the selected satellite image to the other side? I assume the azimuth and angle don't vary much across such short spatial distances (for a given time). If I'm wrong, and they do vary a lot, then maybe, in a future PR, it might be interesting to compute the angle and azimuth for every PV system in the input. But, yeah, maybe that's overkill for now!
# Take the first location, and x and y coordinates are the first and center entries in this array | ||
location = location[0] | ||
# make name of column to pull data from | ||
name = x_y_to_name(x=location[0], y=location[1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit I'm a bit unsure what name
refers to here (and earlier in the code, too). Please could you add a comment, or rename the name
variable to be more explicit? Sorry if I'm just being slow. Is it the name of the GSP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill add a comment, its basically the sun data is saved a x,y locations, so the unique identifier to this is the (x,y) coords.
This doesnt seem that neat, perhaps there is a better way @JackKelly
Co-authored-by: Jack Kelly <[email protected]>
# Conflicts: # nowcasting_dataset/data_sources/pv_data_source.py
# Conflicts: # nowcasting_dataset/data_sources/gsp/gsp_data_source.py # nowcasting_dataset/data_sources/pv_data_source.py # nowcasting_dataset/dataset/datamodule.py # tests/data_sources/get_test_data.py # tests/data_sources/test_pv_data_source.py
Pull Request
Description
Fixes issue #52
How Has This Been Tested?
added unittests
all other unittests
No
Yes
Checklist: