Skip to content

Conversation

RDaxini
Copy link
Member

@RDaxini RDaxini commented Jan 10, 2025

  • Partially addresses Model comparison tables #2329
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

@RDaxini RDaxini added this to the v0.11.3 milestone Jan 10, 2025
@RDaxini RDaxini marked this pull request as ready for review January 14, 2025 18:46
@RDaxini RDaxini marked this pull request as draft January 20, 2025 20:05
@RDaxini
Copy link
Member Author

RDaxini commented Jan 27, 2025

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"?

@cwhanse
Copy link
Member

cwhanse commented Jan 27, 2025

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.

@kandersolar
Copy link
Member

@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.

@RDaxini
Copy link
Member Author

RDaxini commented Jan 28, 2025

@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.)?

@kandersolar
Copy link
Member

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".

@RDaxini
Copy link
Member Author

RDaxini commented Jan 29, 2025

@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

@kandersolar
Copy link
Member

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

@RDaxini RDaxini marked this pull request as ready for review February 11, 2025 17:46
@RDaxini RDaxini changed the title [WIP] Add spectral mismatch model comparison table Add spectral mismatch model comparison table Feb 11, 2025
@kandersolar kandersolar added this to the Someday milestone Jun 6, 2025
@RDaxini
Copy link
Member Author

RDaxini commented Jun 30, 2025

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?

@adriesse
Copy link
Member

adriesse commented Jul 8, 2025

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.

@cwhanse
Copy link
Member

cwhanse commented Jul 24, 2025

@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

@adriesse
Copy link
Member

does anyone strongly object to the first table format shown here?

No objection.

@IoannisSifnaios
Copy link
Contributor

@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...)?

@RDaxini
Copy link
Member Author

RDaxini commented Aug 6, 2025

Converting to draft while I complete the following:

centre ticks
remove background cell shading

if there any other final formatting requests, let me know

@RDaxini RDaxini marked this pull request as draft August 6, 2025 15:45
@kandersolar
Copy link
Member

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.

@RDaxini
Copy link
Member Author

RDaxini commented Aug 11, 2025

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

@RDaxini RDaxini marked this pull request as ready for review September 3, 2025 17:05
@RDaxini
Copy link
Member Author

RDaxini commented Sep 3, 2025

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

Okay, so shall we merge this as is? Are there any objections? Final suggestions?
I will open a separate issue documenting the future improvements that we have discussed here.

@cwhanse
Copy link
Member

cwhanse commented Sep 3, 2025

This PR has ripened. Let's not let it get overripe.

@adriesse
Copy link
Member

adriesse commented Sep 3, 2025

This PR has ripened.

I have the impression the ripening focused on the layout/table more than the introduction. I would really like to see some words of caution in there. Model originators tend not to publish SR curves, and SR curves for different technologies do vary, sometimes a lot. So using one of these models with coefficients for a certain unknown representative of a listed technology carries risk. And without published SR curves, independent validation is pretty hard.

@cwhanse
Copy link
Member

cwhanse commented Sep 3, 2025

This PR has ripened.

I have the impression the ripening focused on the layout/table more than the introduction. I would really like to see some words of caution in there. Model originators tend not to publish SR curves, and SR curves for different technologies do vary, sometimes a lot. So using one of these models with coefficients for a certain unknown representative of a listed technology carries risk. And without published SR curves, independent validation is pretty hard.

I agree, model guidance and comments about validation would be valuable. I think the PR is ripe within the scope the submitter identified: add a table comparing spectral mismatch models.

Can we add the additional content in a follow-on PR?

@RDaxini
Copy link
Member Author

RDaxini commented Sep 4, 2025

I agree that the absence of (SR) data, and IMO also the absence of standardised data collection, processing, and analysis methods, is a limitation of published work when trying to apply published models to different devices/locations/etc. In fact, I agree so much, I am working with a colleague on a small project to address the issue:)

Guidance would be helpful. @adriesse feel free to revise the text directly on this PR and suggest a revision for us to review.

However, the main purpose of this PR was to get the ball rolling with the creation of a new page and a simple table that summarises the functionality we currently have in pvlib-python. Adding extended guidance could take place here, but I think a follow-on PR would be acceptable, possibly even better, for organization and avoiding scope creep here. The latter is my suggestion/preference.

Having said all that, I won't push hard for this to be merged if a core maintainer thinks merging this in its current state would do harm to the repo/community.

@adriesse
Copy link
Member

adriesse commented Sep 5, 2025

I won't stand in the way of a merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants