-
Notifications
You must be signed in to change notification settings - Fork 1.1k
restructure pvlib/spectrum #2136
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
Conversation
update __init__.py rename mismatch.py to spectral_mismatch.py create spectral_irradiance.py, spectral_response.py
resolve circular imports (attempt)
With #2100 a typo was introduced into the list of allowed materials. It is the following:
Can you address it here? It will only require adding some indent (two spaces) to the Sorry for not doing a deep review at that moment. |
Thanks @echedey-ls @cwhanse for highlighting these points
Done. Where do you check to find these error messages? I remember at the time, when adding the extra ``, I had to create a new line, and wondering how/whether to indent it. Since the main tests passed I figured there was not an issue (I was wrong)
Done. Used your definition. What was there originally was just the original text from the |
Since this is a non-user-facing change, do I still need a whatsnew entry? I think it is a significant change, but not one that affects how users interact with pvlib-python (right?), just how we maintain it |
Yes to whatsnew, they also serves as notes for maintainers. |
That's good, makes sense. I have added a whatsnew entry to enhancements. Just let me know if there's anything else |
breaking -> breaking up
I wrote a comment recently in which I elaborated on that #2124 (review) You should get to https://readthedocs.org/projects/pvlib-python/builds/ , head to the latest build of your PR, click on the last box with the command
I hate too that the workflow fails only on critical failures IIRC. |
@echedey-ls thanks for the explanation! And sorry I missed your previous comment:( Seen it now. That's all very helpful 👍🏽 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after a few easy edits. Thanks @RDaxini! I am surprised at how quickly the old mismatch.py
grew to 1000+ lines of code...
@kandersolar I guess we have been quite busy lately 😅 I think I have updated everything according to your latest comments now. Let me know if there's anything else. |
Oh, I guess we should also split up |
@kandersolar split up in what way? Do you mean to reorganise the functions within the
I can do a follow up PR. There is a lot going on in this one already to reorganise the functions, so a separate one for the tests might help keep the changes in neater/compact chunks. |
Yep, like how the structure of |
Restructure tests in line with mismatch.py restructure (pvlib#2136)
Thanks @RDaxini! |
* Split test_spectrum.py files Restructure tests in line with mismatch.py restructure (#2136) * fix conftest import * Update test_mismatch.py add spectrl2_data function to mismatch.py * Update v0.11.1.rst * create pvlib\tests\spectrum\conftest.py
docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.Context and discussion in #2125