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

set meta by pd.DataFrame #94

Merged
merged 4 commits into from
Sep 21, 2018
Merged

Conversation

danielhuppmann
Copy link
Member

@danielhuppmann danielhuppmann commented Sep 20, 2018

Please confirm that this PR has done the following:

  • Tests Added
  • Documentation Added
  • Description in RELEASE_NOTES.md Added

Description of PR

In the existing implementation of set_meta(), passing over a non-valid index (e.g., a pd.DataFrame) was simply ignored without warning or error. This PR is a clean-up in the sense that if there is an index arg, it will be used and an error raised if it cannot be coerced in a meaningful way.

In addition, the function now also takes a pd.DataFrame with columns ['model', 'scenario'] as valid index arg.

@gidden
Copy link
Member

gidden commented Sep 21, 2018

Ok, I think I see the point here, but one question. The test as is has the following lines

    df = pd.DataFrame([
        ['a_model', 'a_scenario', 'a_region1', 1],
    ], columns=['model', 'scenario', 'region', 'col'])
     meta_df.set_meta(meta=0.3, name='meta_values', index=df)

Would the existing implementation work if the user provided the following?

    df = pd.DataFrame([
        ['a_model', 'a_scenario', 'a_region1', 1],
    ], columns=['model', 'scenario', 'region', 'col'])
     meta_df.set_meta(meta=0.3, name='meta_values', index=df[['model', 'scenario']])

_meta = meta if index is None else pd.Series(data=meta, index=index,
name=name)
# check if meta has a valid index and use it for further workflow
if hasattr(meta, 'index') and hasattr(meta.index, 'names') \
Copy link
Member

Choose a reason for hiding this comment

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

do we need to check if index is None here?

@gidden
Copy link
Member

gidden commented Sep 21, 2018

Ok, answered my own question by looking back through. Looks great to me!

@gidden gidden merged commit 06f3aaf into IAMconsortium:master Sep 21, 2018
@danielhuppmann danielhuppmann deleted the set_meta_by_df branch September 21, 2018 09:56
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