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

Mnr/resource #7

Merged
merged 22 commits into from
Sep 27, 2019
Merged

Mnr/resource #7

merged 22 commits into from
Sep 27, 2019

Conversation

MRossol
Copy link
Collaborator

@MRossol MRossol commented Sep 24, 2019

TODO: Tests and final tests, but so far all works and want to get your eyes on this when/if you have some downtime

grantbuster
grantbuster previously approved these changes Sep 26, 2019
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! approved with editorial suggestions.

site_df = pd.DataFrame(columns=gid, index=self.time_index)
site_df.name = ds_name
site_df.index.name = 'time_index'
site_df.loc[:, :] = self[ds_name, :, gid]
Copy link
Member

Choose a reason for hiding this comment

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

For all instances of .loc[:, :], would it be cleaner to initialize the dataframe as df = pd.DataFrame(self[ds_name, :, gid], columns=gid, index=self.time_index)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. this was a lazy change.

@main.command()
@click.option('--dataset', '-d', type=str, required=True,
help='Dataset to extract')
@click.option('--lat_lon', '-ll', nargs=2, type=click.Tuple([float, float]),
Copy link
Member

Choose a reason for hiding this comment

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

Is site data extraction only for lat/lon and never used to retrieve a single GID or list of GIDs? If lat/lon is fast enough i guess we don't need GID extraction, but it could be useful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

gid options added to site and SAM

@click.option('--region', '-r', type=str, default=None,
help='Region to extract')
@click.pass_context
def map(ctx, timestep, dataset, region_col, region):
Copy link
Member

Choose a reason for hiding this comment

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

map is a python special word. This might not cause issues, but maybe it will?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed to timestep to be safe


return tree

def _init_tree(self, tree=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a special method to force the explicit creation of a ckdtree from the meta data? This could be useful for special resource files (Puerto Rico NSRDB, which still has the nsrdb_ filename prefix).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added compute_tree kwarg to WindX, SolarX, and NSRDBX

@grantbuster grantbuster merged commit 3ba9cc0 into master Sep 27, 2019
@grantbuster grantbuster deleted the mnr/resource branch September 27, 2019 14:23
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