restructure pvlib/spectrum

RDaxini commented 2 months ago

[X] Closes #2125
[X] I am familiar with the contributing guidelines
[x] Adds description and name entries in the appropriate "what's new" file in 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`).
[X] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
[x] Pull request is nearly complete and ready for detailed review.
[x] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

Context and discussion in #2125

echedey-ls commented 2 months ago

With #2100 a typo was introduced into the list of allowed materials. It is the following:

/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/2136/pvlib/spectrum/spectral_mismatch.py:docstring of pvlib.spectrum.spectral_mismatch.spectral_factor_firstsolar:40: WARNING: Bullet list ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/2136/pvlib/spectrum/spectral_mismatch.py:docstring of pvlib.spectrum.spectral_mismatch.spectral_factor_firstsolar:37: WARNING: Bullet list ends without a blank line; unexpected unindent.

Can you address it here? It will only require adding some indent (two spaces) to the module. line, no. 159. I can't comment on the moved file since it's not an introduced change.

Sorry for not doing a deep review at that moment.

RDaxini commented 2 months ago

Thanks @echedey-ls @cwhanse for highlighting these points

/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/2136/pvlib/spectrum/spectral_mismatch.py:docstring of pvlib.spectrum.spectral_mismatch.spectral_factor_firstsolar:40: WARNING: Bullet list ends without a blank line; unexpected unindent.
/home/docs/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/2136/pvlib/spectrum/spectral_mismatch.py:docstring of pvlib.spectrum.spectral_mismatch.spectral_factor_firstsolar:37: WARNING: Bullet list ends without a blank line; unexpected unindent.

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)

Can we add a definition of "spectral mismatch"?

Done. Used your definition. What was there originally was just the original text from the mismatch.py file; it was not particularly insightful 😅 I followed a similar style when writing a sentence for the two new files. Let me know if you think any of the other files would benefit from any additional definitions/explanations. Happy to get it done in this same PR.

RDaxini commented 2 months ago

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

cwhanse commented 2 months ago

Yes to whatsnew, they also serves as notes for maintainers.

RDaxini commented 2 months ago

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

echedey-ls commented 2 months ago

Where do you check to find these error messages?

I wrote a comment recently in which I elaborated on that https://github.com/pvlib/pvlib-python/pull/2124#pullrequestreview-2175571173

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 python -m sphinx -T -b html -d _build/doctrees -D language=en . $READTHEDOCS_OUTPUT/html and find all warning, error, critical. Some of them are persistent across PRs.

Since the main tests passed I figured there was not an issue

I hate too that the workflow fails only on critical failures IIRC.

RDaxini commented 2 months ago

@echedey-ls thanks for the explanation! And sorry I missed your previous comment:( Seen it now. That's all very helpful 👍🏽

RDaxini commented 2 months ago

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.

kandersolar commented 1 month ago

Oh, I guess we should also split up pvlib/tests/test_spectrum.py to reflect the new structure. @RDaxini want to add that to this PR, or do a follow-up?

RDaxini commented 1 month ago

Oh, I guess we should also split up pvlib/tests/test_spectrum.py to reflect the new structure.

@kandersolar split up in what way? Do you mean to reorganise the functions within the test_spectrum.py file in ordered groups, or create new files such as test_spectrum_response.py, test_spectrum_irradiance.py? I think the latter sounds like a decent idea, not sure if I understood correctly though. In this case, would a folder "spectrum" with individual test files be a good idea?

@RDaxini want to add that to this PR, or do a follow-up?

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.

kandersolar commented 1 month ago

In this case, would a folder "spectrum" with individual test files be a good idea?

Yep, like how the structure of pvlib/tests/iotools matches that of pvlib/iotools. a 1:1 correspondence of files, with the test ones having test_ at the front of the filenames.

kandersolar commented 1 month ago

Thanks @RDaxini!

pvlib / pvlib-python

restructure pvlib/spectrum #2136