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

Simplify parameterset reading #86

Merged
merged 18 commits into from
Feb 9, 2019
Merged

Conversation

znicholls
Copy link
Collaborator

@znicholls znicholls commented Feb 1, 2019

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <http://link-to-pr.com>`_) Added feature which does something
- (`#XX <http://link-to-pr.com>`_) Fixed bug identified in (`#XX <http://link-to-issue.com>`_)

Simplifies reading of parameterset to make it able to handle string input and also gives root a name rather than being accessed via an empty tuple. This PR also includes a wider update of the docs (cosmetic rather than content).

@znicholls znicholls force-pushed the simplify-parameterset-reading branch from ad88dd6 to 4e25dc0 Compare February 1, 2019 04:14
@znicholls znicholls changed the title WIP: Simplify parameterset reading Simplify parameterset reading Feb 1, 2019
@swillner
Copy link
Member

swillner commented Feb 1, 2019

@znicholls let’s also talk about this in person. I would prefer adding a function ‘str_to_list’ instead of altering the API.

@znicholls
Copy link
Collaborator Author

I would prefer adding a function ‘str_to_list’ instead of altering the API.

str_to_list is a function (well, a decorator but near enough)?

@znicholls
Copy link
Collaborator Author

znicholls commented Feb 4, 2019

After discussion with @swillner @rgieseke:

  • move decorator to a function
  • function throws a warning the first time it has to do type conversion
  • just fixes it in future

@znicholls znicholls self-assigned this Feb 4, 2019
@znicholls znicholls added the wip Work in progress (for PRs) label Feb 4, 2019
@znicholls znicholls force-pushed the simplify-parameterset-reading branch from 6912a63 to 57350fa Compare February 4, 2019 23:45
@znicholls znicholls changed the base branch from master to 90-notebook-dependencies February 5, 2019 04:20
@znicholls znicholls changed the base branch from 90-notebook-dependencies to master February 5, 2019 04:21
@znicholls
Copy link
Collaborator Author

@swillner @rgieseke this is ready to go, it should follow #92 to avoid confusion about the effect of make black

@znicholls znicholls removed the wip Work in progress (for PRs) label Feb 5, 2019
This was referenced Feb 5, 2019
@znicholls znicholls force-pushed the simplify-parameterset-reading branch from 57350fa to 7ba5b20 Compare February 5, 2019 11:21
@znicholls znicholls assigned rgieseke and swillner and unassigned znicholls Feb 7, 2019
Copy link
Member

@swillner swillner left a comment

Choose a reason for hiding this comment

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

I like it! But let's use the simple variant of ensure_input_is_tuple and we are good to go.

openscm/utils.py Outdated Show resolved Hide resolved
@swillner
Copy link
Member

swillner commented Feb 8, 2019

@znicholls Do you give me a ping once you are done here and I will alter the in-code documentation, ok?

@znicholls znicholls force-pushed the simplify-parameterset-reading branch 2 times, most recently from 646abf3 to 390f48c Compare February 8, 2019 23:02
@znicholls
Copy link
Collaborator Author

znicholls commented Feb 8, 2019

Do you give me a ping once you are done here and I will alter the in-code documentation, ok?

I'm done. My preference would be that you make a PR into this branch from a new branch (perhaps called simplify-parameterset-reading or from your own fork). Then it's easier for me to see the changes (to learn how to do the docs better next time) and I don't have to worry about force pushing in future as it won't affect your work.

@swillner
Copy link
Member

swillner commented Feb 8, 2019

Do you give me a ping once you are done here and I will alter the in-code documentation, ok?

I'm done. My preference would be that you make a PR into this branch from a new branch (perhaps called simplify-parameterset-reading or from your own fork). Then it's easier for me to see the changes (to learn how to do the docs better next time) and I don't have to worry about force pushing in future as it won't affect your work.

cool, will do!

@swillner swillner mentioned this pull request Feb 8, 2019
@znicholls
Copy link
Collaborator Author

@swillner so we leave this now until a second reviewer has checked it?

@swillner
Copy link
Member

swillner commented Feb 9, 2019

@swillner so we leave this now until a second reviewer has checked it?

up to you, I relaxed the 2-person requirement and @rgieseke has already seen it when we talked about it in the last meeting...

@znicholls znicholls merged commit 6955969 into master Feb 9, 2019
@znicholls
Copy link
Collaborator Author

Decided

@znicholls znicholls deleted the simplify-parameterset-reading branch February 9, 2019 00:03
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.

None yet

3 participants