-
Notifications
You must be signed in to change notification settings - Fork 9
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
Reeds #18
Reeds #18
Conversation
Main functionality implemented along with a CLI |
Test complete. @mmowers see /reVX/tests/data/reeds/ReEDS_* for the current outputs |
Hi @MRossol, |
@MRossol, awesome thanks, I was on the master branch instead of reeds, so those files weren't there. I see the supply curve in region/class/bin designations in ReEDS_classifications.csv, capacity factor means and standard deviations by timeslice in ReEDS_Timeslice_means.csv and ReEDS_Timeslice_stdevs.csv, and representative profiles in ReEDS_Profiles.h5. Some thoughts/questions (we can discuss on the phone if easier):
|
@mmowers See inline below:
|
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.
Looks good overall but will definitely benefit from a walk through. I assume we'll have other thoughts after tomorrow's regroup but here are some initial thoughts.
reVX/reeds/reeds_classification.py
Outdated
if isinstance(cluster_on, str): | ||
cluster_on = [cluster_on, ] | ||
|
||
data = RPMClusters._normalize_values(rev_table[cluster_on].values, |
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.
Might as well move this method to a common utility repo with the clustering algorithms?
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.
Good call, will do
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.
Added it to the ClusterMethods class
logger = logging.getLogger(__name__) | ||
|
||
|
||
class ReedsProfiles(RepProfiles): |
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 awesome!
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.
You did 90% of the work!
reVX/reeds/reeds_profiles.py
Outdated
profile. | ||
n_profiles : int | ||
Number of representative profiles to save to fout. | ||
bins : None | str | pandas.DataFrame | pandas.Series | dict |
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.
Might want more description on what None does since its default. Also maybe corresponding description of the reg_cols defaults.
reVX/reeds/reeds_timeslices.py
Outdated
raise ReedsValueError(msg) | ||
|
||
index_col = [c for c in timeslice_map.columns | ||
if 'time' in c.lower()] |
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 think the search string should be "datetime" or even "datetimeindex". I'm picturing two columns, one with "datetimeindex" and the other with "timeslice_id" (has "time" in it!). Plus all of the error messages say "datetime".
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'll admit I got lazy here, but I agree, will update
reVX/reeds/reeds_timeslices.py
Outdated
""" | ||
means = [] | ||
stdevs = [] | ||
for s, slice_map in timeslices.groupby('slice'): |
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 see that your slice ID column is just called "slice". Can you add a check for this in _parse_timeslices()
?
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.
Whoops, meant to run the groupby on all remaining columns, will fix
reVX/reeds/reeds_timeslices.py
Outdated
Create ReEDS timeslices from region-bin-class groups and representative | ||
profiles | ||
""" | ||
def __init__(self, rep_profiles, timeslice_map, meta=None, |
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.
We need to verify that the timeslice statistics are calculated from representative profiles and not profiles from all sites in a region/timeslice. I'm totally not sure, but that would be a high level misunderstanding.
Looking at @mmowers' doc, it would appear this is an open question.
stdevs = pd.concat(stdevs, axis=1).T | ||
stdevs.index.name = 'timeslice' | ||
|
||
return means, stdevs |
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.
Clean method! Nice!
logger = logging.getLogger(__name__) | ||
|
||
|
||
class ReedsClassifier: |
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 think this looks good but I definitely would benefit from a walk through :)
Michael and Grant code review notes: Classification items:
Timeslices:
|
@grantbuster I would love your eyes on these two formatting methods in ReedsTimeslices as they are really slow but I'm not sure how else to speed them up without a for loop... The timeslice CLI entry needs to be updated on your branch, hopefully we can do that and merge it monday... |
7e935ed
to
3608af5
Compare
Call with Matt: |
f68d5a9
to
387807f
Compare
83d3e2c
to
4a7a6c3
Compare
remove redundant logging functions
protect hour against "overflow"
…nteger, and process pool with spawn option
…eds specifications
Merging this pull request since we've tested thoroughly. |
reV to ReEDS pipeline w/ CLI