pvlib
Add spectral mismatch model comparison table
- [X] Partially addresses #2329
- [X] I am familiar with the contributing guidelines
- [ ] ~Tests added~
- [ ] ~Updates entries in
docs/sphinx/source/referencefor API changes.~ - [ ] Adds description and name entries in the appropriate "what's new" file in
docs/sphinx/source/whatsnewfor 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.
Questions: Should the inputs be linked to the pvlib functions that can calculate these inputs? I think it would be convenient for readers, but I also think it would a) clutter the table with long links, and b) in the case of multiple options, introduce bias if we were to select one. Overall, I lean away from linking inside the table. Thoughts? Alternatives? Maybe a list of links at the bottom, or at least a sentence or two informing users of pvlib's general capability to calculate inputs
How many cell technologies is too many to list? I think after 2 or 3, it might be best just to write "multiple"?
I don't think links to the input definitions add much value. It would be easier to read the list of inputs if the list had one input on each line, rather than a comma-separate text paragraph.
It doesn't seem very helpful when most "Cell technology" fields have a value of "Multiple". I'd use the vertical real estate to list all the cell technologies, except for 'sapm' and 'mismatch_field' which aren't specific to a cell type. The SAPM model is specific to a module product, not to a cell type.
I think "Data source" doesn't add much here. The primary use is for a modeler looking to select a model. Data used for development and validation can be relegated to the references.
@RDaxini it looks like you're thinking to create a single page to house all comparison tables, is that right?
In #2329 I was imagining these tables would live in pages dedicated to the relevant modeling topic. For example, the table in this PR would be one component of a broader page explaining pvlib's functionality related to spectrum and spectral mismatch. Similarly, the transposition model table would be in a page talking about the irradiance models. I still think that's a good approach, although of course I am interested in hearing opposing viewpoints.
@kandersolar you're too fast again, haha. I went for single page originally because we did not have subsections at that time, but I think 0be2e46 just before your comment should have fixed this in line with your suggestion. We now have one main subsection called model comparisons, and then there will be individual subsubsections explaining the functionality and comparing models. Have a look, let me know if that's what you had in mind. Or did you mean a whole subsection for "spectrum", another for "irradiance", rather than a subsection called "model comparison" (can be renamed) with subsections for those topics (spectrum, irradiance, etc.)?
Ok I see, nice. I suggest merging the Model comparisons section with the existing Modeling topics section. Also this new page's filename should be renamed to something involving "spectrum".
@kandersolar, how about 7e79344? I left it as just "spectrum" to keep it general for now as we could potentially add more content to that page. Spectral corrections could be a single subsection on the "spectrum" page. I can fix up these details later too, I don't have any strong preference currently.
Aside, related: the user guide folder could benefit with some organization of the files, what do you think? I was considering opening a separate issue to seek opinions on categorizing those files into folders, perhaps folders aligned with the subsections perhaps... not urgent/major but the thought came to mind while working on this
I am +1 to where it's going now.
opening a separate issue to seek opinions on categorizing those files into folders, perhaps folders aligned with the subsections
+1 to this as well
Can we work in a shout-out to: https://datahub.duramat.org/dataset/module-sr-library?
Also wondering about chronological ordering of the models so new ones will get added at the end.
In addition to @kandersolar suggestion I want to suggest:
- Instead of yes, use "x", unicode ticks ✓ or emojis (I prefer unicode ticks for this)
- Reorder the types columns, so we have all Si types near the same place (start as it is the most common?)
- Add a nested column index the same way
Generic coefficient values available?has the types, but for the inputs. It would be easier to find a model for a given set of inputs. - Can we shorten
Generic coefficient values available?/Generic parameters available?to something alongDefault parameters for tech/Default tech parameters/Default values for? Just thinking out loud, I'm okay as it currently is.
Also wondering about chronological ordering of the models so new ones will get added at the end.
- Instead of chrono order, I prefer to group them by the common inputs/default tech params. Kind of this recommendatio for statistics, but for the table. Make it easy to read.
If you want to change the widths of each column, you may need to use the .. table directive:
- https://github.com/pvlib/pvlib-python/blob/2ca40aa377d0871905f3c12f7ab0de0d437e2e21/docs/examples/floating-pv/plot_floating_pv_cell_temperature.py#L56
- https://docutils.sourceforge.io/docs/ref/rst/directives.html#table (for other rST attributes)
Point 4. makes this table too wide:
Details
+-----------------------------------------------------+---------------------------------------------------------------------------------+-----------------------------------------------------+------------------+
| | Inputs | Default parameters for tech | |
| Model +------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+ Reference & year |
| | airmass_relative | airmass_absolute | precipitable_water | clearsky_index | aod | a-Si | mono-Si | poly-Si | CdTe | CIGS | perovskite | |
+=====================================================+==================+==================+====================+================+=====+======+=========+=========+======+======+============+==================+
| :py:func:`Caballero <spectral_factor_caballero>` | | ✓ | ✓ | | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | [2]_, 2018 |
+-----------------------------------------------------+------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+------------------+
| :py:func:`First Solar <spectral_factor_firstsolar>` | | ✓ | ✓ | | | | | ✓ | ✓ | | | [3]_, 2016 |
+-----------------------------------------------------+------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+------------------+
| :py:func:`JRC <spectral_factor_jrc>` | ✓ | | | ✓ | | | | ✓ | ✓ | | | [6]_, 2008 |
+-----------------------------------------------------+------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+------------------+
| :py:func:`PVSPEC <spectral_factor_pvspec>` | | ✓ | | ✓ | | ✓ | ✓ | ✓ | ✓ | | | [5]_, 2020 |
+-----------------------------------------------------+------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+------------------+
| :py:func:`SAPM <spectral_factor_sapm>` | | ✓ | | | | | | ✓ | ✓ | | | [4]_, 2004 |
+-----------------------------------------------------+------------------+------------------+--------------------+----------------+-----+------+---------+---------+------+------+------------+------------------+
An idea to make it work would be to transpose it:
Details
.. currentmodule:: pvlib.spectrum
+--------------------------------------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| Model | :py:func:`Caballero <spectral_factor_caballero>` | :py:func:`First Solar <spectral_factor_firstsolar>` | :py:func:`JRC <spectral_factor_jrc>` | :py:func:`PVSPEC <spectral_factor_pvspec>` | :py:func:`SAPM <spectral_factor_sapm>` |
+-----------------------------+--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | airmass_relative | | | ✓ | | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | airmass_absolute | ✓ | ✓ | | ✓ | ✓ |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| Inputs | precipitable_water | ✓ | ✓ | | | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | clearsky_index | | | ✓ | ✓ | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | aod | ✓ | | | | |
+-----------------------------+--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | a-Si | ✓ | | | ✓ | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | mono-Si | ✓ | | | ✓ | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | poly-Si | ✓ | ✓ | ✓ | ✓ | |
| Default parameters for tech +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | CdTe | ✓ | ✓ | ✓ | ✓ | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | CIGS | ✓ | | | | |
| +--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| | perovskite | ✓ | | | | |
+-----------------------------+--------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
| Reference & year | [2]_, 2018 | [3]_, 2016 | [6]_, 2008 | [5]_, 2020 | [4]_, 2004 |
+--------------------------------------------------+--------------------------------------------------+-----------------------------------------------------+--------------------------------------+--------------------------------------------+----------------------------------------+
This table does not have point 5 applied.
Spreadsheet file: pv-dax-spectrum-comparison.ods
Btw, I'm using this generator here: https://www.tablesgenerator.com/text_tables
Instead of yes, use "x", unicode ticks ✓ or emojis (I prefer unicode ticks for this)
I used the ticks at first too, but my editor did not display them in monospace font, so it messed up the table alignment. I switched to "yes" out of irritation. I am +1 to the unicode ticks, as long as someone else makes them work ;)
Instead of yes, use "x", unicode ticks ✓ or emojis (I prefer unicode ticks for this)
I used the ticks at first too, but my editor did not display them in monospace font, so it messed up the table alignment. I switched to "yes" out of irritation. I am +1 to the unicode ticks, as long as someone else makes them work ;)
I did have that mismatch too. DejaVu Sans Mono fixes it and looks great. Uniglyph does too, but it's too pixeled for me. I had DejaVu already installed in Windows, IDK if that was past me, some program I had installed or Microsoft getting something right.
I mildly prefer the "models in column" format. I think it is more likely to add new models (rows) than to add columns.
"Default parameters" > "Generic parameters" for me.
I also like the link to pvlib terms, as long as each term has its own row so I'm not reading a paragraph of variable names.
Can we work in a shout-out to: https://datahub.duramat.org/dataset/module-sr-library?
@adriesse, a suggestion: 298caa8. Your thoughts?
(note Ref. was changed to Reference in the next commit)
Can we work in a shout-out to: https://datahub.duramat.org/dataset/module-sr-library?
@adriesse, a suggestion: 298caa8. Your thoughts?
Having reread the whole page now, I think it would benefit from an introduction that clearly separates functions/models that actually use spectrally-resolved data vs. those that don't. Then I think the reference will also fall into place more naturally.
Lots of good suggestions here.
I could be convinced otherwise, but atm I'm not 100% on board with ticking off variables. Is there a chance we imply more ticks => better, ie more variables => better? We know that is not necessarily the case since there is variation in importance among individual variables for different PV types, climates, etc., and mixed value in their use in conjunction with one another. It's similar with ticking off modules since some modules may be more/less important based on age/technology/etc but... I don't think that is as big of an issue though.
So maybe we go with Kevin's formatting suggestion, but with ticks rather than "yes" as suggested by Echedey?
I mildly prefer the "models in column" format. I think it is more likely to add new models (rows) than to add columns.
+1, adding rows will fit better on the page, so one model per row sounds good.
each term has its own row so I'm not reading a paragraph of variable names.
You mean a new line within each table cell, or a full distinct table row for each variable?
@RDaxini regarding
I could be convinced otherwise, but atm I'm not 100% on board with ticking off variables. Is there a chance we imply more ticks => better, ie more variables => better? We know that is not necessarily the case since there is variation in importance among individual variables for different PV types, climates, etc., and mixed value in their use in conjunction with one another. It's similar with ticking off modules since some modules may be more/less important based on age/technology/etc but... I don't think that is as big of an issue though.
I now regret small recommendations I wrote in past PRs. I don't think pvlib is the place to put not entirely factual information, cause it can get misunderstood and sometimes misses a bigger picture of the modelling. But feel free to study and publish that in a paper, it may make for a relevant piece of knowledge. Great observation.
On the other hand, if you think how to format is holding back this PR for too long, it may be time to:
- add them all so we can all see each one (and vote)
- go ahead with the one you like the most and let's see if there's something to be improved
Just an opinion update: I strongly believe putting each model input in a column is beneficial, so that libraries like DataTables (also this repo) can let users sort by inputs. My opinion was not so strong in the past.
New build: https://pvlib-python--2353.org.readthedocs.build/en/2353/user_guide/modeling_topics/spectrum.html (note: no idea why but when I open this link, after it has opened, I have to hit refresh for the rendered page to update to the latest version...)
Included three examples based on suggestions from @kandersolar and @echedey-ls (thanks)
Mild preferences for the third table version over the first. Either are better than the 2nd. The one disadvantage of the third table is that if pvlib adds a few more spectral models, the table has to expand horizontally.
The one disadvantage of the third table is that if pvlib adds a few more spectral models, the table has to expand horizontally.
I lean towards the first table and this is one of main the reasons why. I also think the readability logic makes more sense going from left to right, starting with the model (left) and then introducing info about the model (right), rather than top to bottom.
Re new models, we can expect at least one new model from First Solar soon. Not sure what the current status is but we have been hearing about it for at least a year now. I think it is reasonable to expect new models for bifacial and/or tandems to be published in the future too.
I prefer the first table and would suggest putting the year and reference in the first column and then sorting by date. Maybe suppress the shading of alternate rows.
The text is not always clear, for example the last sentence. I would suggest a structure like:
- spectra and spectral responses: functions and data
- spectral mismatch meaning and calculation
- spectral factor models (= spectra mismatch estimators?)
I prefer the first table and would suggest putting the year and reference in the first column and then sorting by date. Maybe suppress the shading of alternate rows.
Happy to do this once we are all agreed on a table format.
Is agreeing on a table format and implementing finishing touches (such as the quotes suggestions) all that is required now to complete this PR?
It doesn't seem very helpful when most "Cell technology" fields have a value of "Multiple". I'd use the vertical real estate to list all the cell technologies, except for 'sapm' and 'mismatch_field' which aren't specific to a cell type. The SAPM model is specific to a module product, not to a cell type.
Coming back to this point: the existing First Solar model provides parameters for two specific CdTe products.
I would hazard a guess that the vast majority (or maybe even all) of the model parameters are for specific products.
@pvlib/pvlib-maintainer does anyone strongly object to the first table format shown here? https://pvlib-python--2353.org.readthedocs.build/en/2353/user_guide/modeling_topics/spectrum.html
does anyone strongly object to the first table format shown here?
No objection.
@pvlib/pvlib-maintainer does anyone strongly object to the first table format shown here?
No objection, but maybe we could put the tickmarks closer to the center of each cell in order to avoid anybody thinking they refer only to the first parameter (not sure if this is easy though...)?
Converting to draft while I complete the following:
centre ticks remove background cell shading
if there any other final formatting requests, let me know
maybe we could put the tickmarks closer to the center of each cell
Maybe I am wrong, but I doubt there is a clean way to do this. I think we would have to resort to something like custom CSS or even hackery with NBSP characters (neither of which is worth it IMHO). I suggest saving that for a follow-up PR if someone is motivated to look for a nice solution.
maybe we could put the tickmarks closer to the center of each cell
Maybe I am wrong, but I doubt there is a clean way to do this. I think we would have to resort to something like custom CSS or even hackery with NBSP characters (neither of which is worth it IMHO).
That's what I though too after checking online, which is why I converted this to a draft. I think it is the same case with the row shading too, unless anyone has any other ideas on how to adjust the row shading?
I suggest saving that for a follow-up PR if someone is motivated to look for a nice solution.
+1 I think it would be good to get this out there. We can always fine tune in future work