make iotools package #272
Merged
make iotools package #272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added a reader for PVSyst files: This also shows the challenge in getting metadata from CSV files of different sources. I hope this helps to define
I think we shall agree on the general structure before improving each reader (documentation / testing) |
I think we essentially agreed on the general structure in #261. Is there some part of the structure that you disagree with? At this point, I think our time is better spent focusing on implementing new readers. |
|
@dacoex thanks so much for getting this off the ground. And to @wholmgren for setting up the branch for development. I'll get the surfrad reader uploaded, and copy the tmy readers over. Great to see the progress! |
|
Great! Be sure to fetch and checkout the new io branch from the main github organization before adding your surfrad reader. I already moved the Tmy readers so it shouldn't be necessary to touch them. Sorry Cliff and @dacoex for the git confusion and my poor explanations. |
|
There are 113 errors:
|
What are you really trying to say stickler? Just spit it out, don't sugar coat it |
|
I think this is ready to be reconsidered after almost 2 years. In short:
The old functions remain but emit deprecation warnings. I marked them for removal in 0.7 for now. Here's a link to the rendered API documentation: https://pvlib-python.readthedocs.io/en/io/api.html#io-tools We'll make a few PRs for adding U Oregon SRML and Surfrad in the next couple of weeks. We'll make the PRs to the I'm ignoring the majority of the Stickler errors since they're not really new and I don't care to take the time to fix them. Unfortunately I don't know how to turn them off for this PR. At least it didn't inline the comments! |
cwhanse
approved these changes
Sep 21, 2018
This was referenced Sep 21, 2018
Closed
Closed
|
@mikofski any new thoughts/objections on package structure 2 years later? (deleted stickler comment) |
|
TL;DR: I like the new structure. Although I think |
|
@mikofski thanks for the review. I think our discussion here was the start of the |
|
There are 102 errors:
|
|
Moved the API declarations to sphinx docs say that we'd need to convert the "simple tables" to "grid tables" to be able to wrap lines. not worth the effort. I think this is ready to merge. |
|
Sorry, that statement from 2 years ago contradicts my review here. I guess I'm ambivalent. But I respect your judgement |
|
Hearing no objections, I will go ahead and merge this. We can easily change (or even revert) through the next release. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I made a new
iobranch to try to streamline the process of multiple users contributing their new io tools and to make it easier for all of us to test it them without hurting the master branch. I also went ahead and moved the tmy readers into the package and updated the tests accordingly. I named the packageiotoolssince Cliff expressed a preference for that name, but it's not yet set in stone. I think this is mostly consistent with what we talked about in #261 and #29. Let me know if I missed something concerning the structure.Please make your new IO tools pull requests against this branch rather than master.
Finally, I'm not going to merge into master until we're ready to make a series of changes to the API. I want to do at least 0.4 release with non-API breaking changes before then.
Closes #29. Closes #261.