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

Multi year resource handlers #17

Merged
merged 5 commits into from
Jul 8, 2020
Merged

Multi year resource handlers #17

merged 5 commits into from
Jul 8, 2020

Conversation

MRossol
Copy link
Collaborator

@MRossol MRossol commented Jul 7, 2020

Feature Request from the guys who created the new US_Wave data (on eagle and HSDS). They want a single entry point to be able to pull multi-year profiles. Was a fund implementation, I'm particularly happy with the ability to pass "year" values in the axis 0 slice as a string or list of strings

@MRossol MRossol added the feature New feature or request label Jul 7, 2020
@MRossol MRossol requested a review from grantbuster July 7, 2020 21:23
Add multi-year cli
update docs
@MRossol
Copy link
Collaborator Author

MRossol commented Jul 8, 2020

WAPP 10343 20.01.01

Copy link
Member

@grantbuster grantbuster left a comment

Choose a reason for hiding this comment

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

Looks great overall! I think we should consider a major file reorg though. I need a flow chart to keep everything straight. For example, these files groups are very similar:

mutli_year_resource, renewable_resource, resource, resource_extraction

resource_cli, solar_cli, wind_cli

Could we rename resource to h5_resource? Just the filename i mean. Maybe change the cli suffix to a prefix? Make subfolders to the "extraction" utilities vs. the handlers?

Basically if I am super confused by all these files I'm afraid the public will have no idea how to use anything.

@@ -0,0 +1,834 @@
# -*- coding: utf-8 -*-
"""
Classes to handle resource accross multiple files
Copy link
Member

Choose a reason for hiding this comment

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

This file is misspelled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DOPE!

return year in self.years

@property
def attrs(self):
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this to the normal Resource class? Can't tell you how many times i've wanted it instead of calling .get_attrs()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

@MRossol
Copy link
Collaborator Author

MRossol commented Jul 8, 2020

What if we move resource_extraction and the cli files to a sub_package since they are all related?

@grantbuster
Copy link
Member

I think that would go a long way, yes. Maybe add a readme to that subpackage too? We can add the link to the docs but it would be good to have a one sentence explanation of what are these files and why would you want to use them vs. the handlers.

@MRossol
Copy link
Collaborator Author

MRossol commented Jul 8, 2020

Okay, how does that look?

Also, there was a global_attrs property, I also added attrs as a further alias

@grantbuster
Copy link
Member

I think its a lot better, what do you think?

@MRossol MRossol merged commit 606a393 into master Jul 8, 2020
@MRossol MRossol deleted the mnr/multi_year_resource branch July 8, 2020 17:42
github-actions bot pushed a commit that referenced this pull request Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants