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

add function concat() #190

Merged
merged 7 commits into from
Feb 12, 2019
Merged

add function concat() #190

merged 7 commits into from
Feb 12, 2019

Conversation

gidden
Copy link
Member

@gidden gidden commented Feb 6, 2019

Please confirm that this PR has done the following:

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

Description of PR

This adds a concat() function a la pandas utilizing our append() function

pyam/utils.py Outdated Show resolved Hide resolved
pyam/utils.py Outdated Show resolved Hide resolved
pyam/utils.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member Author

gidden commented Feb 6, 2019

No tests yet, mea culpa

@coveralls
Copy link

coveralls commented Feb 6, 2019

Coverage Status

Coverage increased (+0.09%) to 84.197% when pulling ccb7938 on gidden:concat into 213f1d6 on IAMconsortium:master.

@gidden
Copy link
Member Author

gidden commented Feb 8, 2019

ok, ready for review

@gidden
Copy link
Member Author

gidden commented Feb 8, 2019

seems like an erroneous test failure, restarting

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

thanks @gidden, really nice new feature!
As commented, append() is more powerful and takes multiple input types.

The new function concat() could easily resolve #168 by checking in the IamDataFrame constructor if data is a list and calling pyam.concat(). Then read_files() would ever only have to read one file at a time.

pyam/core.py Outdated Show resolved Hide resolved
@gidden
Copy link
Member Author

gidden commented Feb 11, 2019

Hey @danielhuppmann, I was thinking about this and I am not sure what the best strategy is here. I understand trying to make things as "easy to use as possible", but we also need to balance that with a well designed and extendable code base. I am wary to try to start casting things all over the place in the internals (even though it sounds like we have already tried to start doing so).

It would seems much more reasonable from a design perspective to have users either explicitly use a constructor or some other explicit casting function (to_iam_df()). Our internals could then always assume that they are getting a IamDataFrame or subclass and anything else is an error.

Just as an example, the code as written now fails the test I wrote if I take out this type check (because it doesn't explicitly cast the string to a df and then fails). Happy to talk more as you wish.

@danielhuppmann
Copy link
Member

I see your point in principle, @gidden, but I disagree when it comes to operations like x + y = z, where the types can easily be guessed.

Three arguments:

  • We have only one data type in pyam, so I think it's a fair assumption to try to cast reasonable inputs to that type where reasonable and where the "intermediate" IamDataFrame won't realistically be reused (like creating an IamDataFrame from a list of files).
  • From a user perspective, the current implementation df.append('<some_path>') is more pythony than df.append(pyam.IamDataFrame('<some_path>')) (and yields the same error message). The same logic applies to pyam.concat(), I think.
  • From an internals design perspective, checking whether the input is an IamDataFrame vs. casting the arg is the same level of complexity.

Re the failing unit test - yes, true, the first item of the list will also have to be cast. But then, calling pyam.concat('foo') will try to read a file f and return a ValueError instead of a TypeError- which I would consider equally valid here.

@gidden
Copy link
Member Author

gidden commented Feb 11, 2019

Ok, per an in-person discussion, we agreed to leave this existing PR as is and move the discussion to an issue for future consideration.

@gidden
Copy link
Member Author

gidden commented Feb 12, 2019

this failure has to do with travis' windows ci, which is still in beta. we still have windows covered on appveyor, so this is able to be pulled in. @danielhuppmann mind doing the honors?

@danielhuppmann
Copy link
Member

happy to oblige, @gidden!

@danielhuppmann danielhuppmann merged commit b665608 into IAMconsortium:master Feb 12, 2019
@gidden gidden deleted the concat branch June 15, 2022 11:26
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.

4 participants