[BUG]: don't infer spectral model in ModelChain

[echedey-ls]

• Closes Inferring spectral model returns 'first_solar' when not expected #2017
• 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.

Fix #2017 - note the approach may have been different. In this case, spectral inference has been deferred until weather is known, in order to take its available parameters for the inference.

Tested for script:

Code cleaned up from #2017

import pandas as pd
import pvlib


from pvlib.modelchain import ModelChain
from pvlib.location import Location
from pvlib.pvsystem import PVSystem
from pvlib.temperature import TEMPERATURE_MODEL_PARAMETERS


location = Location(
    latitude=40.30117520975112,
    longitude=-3.6973221576701967,
    tz="Europe/Madrid",
    altitude=660,
    name="Ormazabal Getafe",
)
modules = pvlib.pvsystem.retrieve_sam("CECMod")
inverters = pvlib.pvsystem.retrieve_sam("CECInverter")
module = modules["JA_Solar_JAP72S01_330_SC"]
inverter = inverters["SMA_America__STP_33_US_41__480V_"]
temp_par = TEMPERATURE_MODEL_PARAMETERS["sapm"]["open_rack_glass_glass"]
system = PVSystem(
    surface_tilt=19,
    surface_azimuth=170,
    module_parameters=module,
    inverter_parameters=inverter,
    temperature_model_parameters=temp_par,
    modules_per_string=17,
    strings_per_inverter=6,
)
modelchain = ModelChain(system, location, aoi_model="physical")

times = pd.date_range(
    start="2023-06-01", end="2023-12-15", freq="h", tz=location.tz
)
solar_position = location.get_solarposition(times)
clearsky = location.get_clearsky(times)

modelchain.run_model(clearsky)

Still haven't added tests, please let me know what you think about the behaviour first.

[cwhanse]

Maybe we're trying too hard to help. I'm inclined to default to the no_loss model if parameters are not found, regardless of Technology, Material or weather.

Or just do away with the inference altogether and set to no_loss if spectral_model=None

[echedey-ls]

Or just do away with the inference altogether and set to no_loss if spectral_model=None

I've defaulted to spectral_model="no_loss" and documented the infer_spectral_model as a helper function so the user can opt in to inference. I will write the tests now.
Nvm, it didn't work as many tests expect SAPM to be used whenever possible, so I've documented how it works when trying to infer without weather and that's it.

[echedey-ls]

L256 codecov warning in the diff is related to a PEP8 line change and the one in L910 is related to a different issue, #2259.

[echedey-ls]

Are this PR changes desired in pvlib or is another way of handling / changing this behaviour desired?

[cwhanse]

@echedey-ls I'm more inclined to remove the inference altogether, rather than continue to check the module parameters and/or the weather. This changed code preserves the old behavior, which is somewhat murky and was not well-documented (e.g., which values of "Technology" and "Material" led to which spectral models?) Also, the added inspection of weather is not used by the "run_model_" routines, so a user may expect that the ModelChain will handle all that spectral stuff for him, only to find out later that there was no spectral adjustment in the model results.

These are the reasons I think that spectral_model=None should simply default to the 'no_loss' model, instead of the current behavior.

[echedey-ls]

@cwhanse I strongly agree with that. Shall I remove the function that makes the explicit inference too? I would keep it so users can explicitly make the inference, it's easier to expand in the future and may even return a collection of models the ModelChain + weather can be used for.

[AdamRJensen]

@cwhanse I strongly agree with that. Shall I remove the function that makes the explicit inference too? I would keep it so users can explicitly make the inference, it's easier to expand in the future and may even return a collection of models the ModelChain + weather can be used for.

I would vote yes.

[cwhanse]

I think we keep the inference, but return 'no_loss' unless parameters for the 'sandia' or 'first_solar' models are found. I would not inspect 'weather' in that inference. If a user passes the 'first_solar' parameters with 'module_parameters' they should know enough to include 'precipitable_water' in 'weather'.

[cwhanse]

@echedey-ls I can pick up this PR and move it forward, if you'd like.

[echedey-ls]

@echedey-ls I can pick up this PR and move it forward, if you'd like.

Yes please, at this point I'm a little lost on what would the next steps be. Feel free to open a new branch and close this PR - whatever is comfier for you.

[kandersolar]

One remaining thing, remove this docs entry: https://github.com/pvlib/pvlib-python/blob/main/docs/sphinx/source/reference/modelchain.rst?plain=1#L115

[kandersolar]

@cwhanse in a previous comment you said:

I think we keep the inference, but return 'no_loss' unless parameters for the 'sandia' or 'first_solar' models are found.

I don't think this PR currently does that. Just checking: we want to move forward with no inference at all?

[cwhanse]

@cwhanse in a previous comment you said:

I think we keep the inference, but return 'no_loss' unless parameters for the 'sandia' or 'first_solar' models are found.

I don't think this PR currently does that. Just checking: we want to move forward with no inference at all?

I think it's better not to infer, and if I read it correctly @AdamRJensen concurs. Setting a default of 'no_loss' solves the "I don't know what to do with this spectral model" problem that many new users have, and it has the virtue of being explicit, where the old "infer" technique may do something the user didn't intend.

I don't think it is wise to keep the infer_spectral_model method if it's not being used.

[kandersolar]

Thanks @echedey-ls and @cwhanse!