Skip to content

Consider replacing inverter methods in PVSystem with a single method with a kwarg #998

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

Closed
cwhanse opened this issue Jul 7, 2020 · 10 comments · Fixed by #1147
Closed

Consider replacing inverter methods in PVSystem with a single method with a kwarg #998

cwhanse opened this issue Jul 7, 2020 · 10 comments · Fixed by #1147
Labels
Milestone

Comments

@cwhanse
Copy link
Member

cwhanse commented Jul 7, 2020

#886 moved inverter-related functions from pvsystem.py to inverter.py. Methods PVSystem.snlinverter and PVsystem.adrinverter currently wrap inverter.sandia and inverter.adr, respectively, which previously were pvsystem.snlinverter and pvsystem.adrinverter.

Perhaps renaming the functions creates potential for confusion.

It may be an improved API to replace PVSystem.snlinverter, PVSystem.adrinverter and PVSystem.pvwatts_ac with a single method PVSystem.dc_to_ac or similar, with a kwarg model='sandia' for example.

Opening for discussion.

@wholmgren
Copy link
Member

I’m all for this.

The other wrapper methods for multiple models use a get_ prefix so might want to consider that for consistency.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 4, 2020

PVSystem.ac_from_dc, PVSystem.dc_to_ac, PVSystem.get_ac or ? Looking for ideas.

@cwhanse cwhanse added this to the 0.9.0 milestone Sep 6, 2020
@wholmgren
Copy link
Member

@cwhanse we've added a couple more inverter models (the multi variants) to PVSystem in the 0.9.0 development cycle. I'm thinking we should move forward on this issue before they're released and we need to go through more deprecations.

As for name, I think PVSystem.get_ac or PVSystem.get_ac_power is most consistent with the other wrapper methods, but I also like the other options you proposed.

@wholmgren wholmgren added the api label Jan 21, 2021
@cwhanse
Copy link
Member Author

cwhanse commented Jan 21, 2021

Agree. I'm partial to get_ac with how=<method name>

I can work on this.

@wholmgren
Copy link
Member

How should we account for the lack of voltage data with pvwatts? I guess we can use a kwarg that's required for the non-pvwatts methods. Alternatively we could require the user to pass an argument that would be ignored for the pvwatts method. Neither sounds great.

@cwhanse
Copy link
Member Author

cwhanse commented Jan 22, 2021

ModelChain.get_ac wouldn't have that issue, I think, since it can inspect ModelChain.results.dc and get what's needed.

For PVSystem.get_ac(pdc, vdc=None) is similar to what I see done in e.g. scipy.optimize.minimize where many methods are wrapped, and some kwargs are needed for some but not all methods.

@cwhanse
Copy link
Member Author

cwhanse commented Jan 22, 2021

Reaction to this PVSystem.method? It assumes the current PVSystem inverter methods will be deprecated.

    def get_ac(self, p_dc, v_dc=None, model=None):
        r"""Calculates AC power from p_dc using the inverter model indicated
        by model and self.inverter_parameters.

        Parameter model must be one of 'sandia', 'adr', or 'pvwatts'.

        Parameter v_dc is required for model='sandia' or model='adr'.

        """
        model = model.lower()
        if model not in ['sandia', 'pvwatts', 'adr']:
            raise ValueError(
                model + ' is not a valid AC power model.',
                ' model must be one of "sandia", "adr" or "pvwatts"')
        if model == 'pvwatts':
            kwargs = _build_kwargs(['eta_inv_nom', 'eta_inv_ref'],
                                   self.inverter_parameters)
        if self.num_arrays > 1:
            if model not in ['sandia', 'pvwatts']:
                raise ValueError(
                    model + ' is not a valid AC power model.',
                    ' model must be one of "sandia", "adr" or "pvwatts"')
            p_dc = self._validate_per_array(p_dc)
            if v_dc is not None:
                v_dc = self._validate_per_array(v_dc)
            if model == 'sandia':
                return inverter.sandia_multi(v_dc, p_dc,
                                             self.inverter_parameters)
            elif model == 'pvwatts':
                return inverter.pvwatts_multi(
                    p_dc, self.inverter_parameters['pdc0'], **kwargs)
        else:  # single array
            if model == 'sandia':
                return inverter.sandia(v_dc, p_dc, self.inverter_parameters)
            elif model == 'pvwatts':
                return inverter.pvwatts(p_dc, self.inverter_parameters['pdc0'],
                                        **kwargs)
            elif model == 'adr':
                return self.adrinverter(v_dc, p_dc)

@cwhanse
Copy link
Member Author

cwhanse commented Jan 22, 2021

I think the laddered if/else is clear but not elegant. It could be more compact (and probably less clear) if we accepted 'sandia_mult' and 'pvwatts_multi' as model values, but, I think it's easier on the user to build the multi-array inverter model selection into this method.

@cwhanse
Copy link
Member Author

cwhanse commented Jan 22, 2021

ModelChain.get_ac presents a different problem. Currently, ModelChain accepts the 'ac_model' parameter as a string which is used to set the attribute ModelChain.ac_model to a ModelChain method for the inverter model.

What is expected from ModelChain.get_ac?

  • sets the ModelChain.ac_model attribute, and the .run_model method still has a self.ac_model() statement.
  • sets the ModelChain.results.ac attribute, in which case self.ac_model() is replaced by self.get_ac()

@wholmgren
Copy link
Member

Method generally looks good to me. A few suggestions:

  • model is required, so it should be an arg or a kwarg with a non-None default value.
  • I think the _validate_per_array calls belong outside the if block.
  • replace the if model not in ['sandia', 'pvwatts'] with an else block that raises the same ValueError

I think it's easier on the user to build the multi-array inverter model selection into this method.

I'm ok with that. Would you apply the same logic to the modelchain layer? That would be my preference.

ModelChain.get_ac presents a different problem.

We don't necessarily need to do that at the same time. Those ModelChain methods should be cleaned up but I'm less concerned about them. I don't think anyone uses them directly and I wouldn't have concerns about changing them without deprecation.

As for actual implementation, one way might be to set the attribute equal to a partial(self.system.get_ac, model=ac_model).

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

Successfully merging a pull request may close this issue.

2 participants