Skip to content

Implement PVSystem.get_ac #1147

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

Merged
merged 10 commits into from
Jan 27, 2021
Merged

Implement PVSystem.get_ac #1147

merged 10 commits into from
Jan 27, 2021

Conversation

cwhanse
Copy link
Member

@cwhanse cwhanse commented Jan 25, 2021

Anticipates deprecation of the separate inverter methods PVSystem.snlinverter, .adrinverter, .sandia_multi, .pvwatts, and .pvwatts_multi after ModelChain.ac_model is reworked.

kwargs = _build_kwargs(['eta_inv_nom', 'eta_inv_ref'],
self.inverter_parameters)
if self.num_arrays > 1:
p_dc = self._validate_per_array(p_dc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line assuming we move the validation call above the if model block

"""
model = model.lower()
if model == 'sandia':
if self.num_arrays > 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is used in 2 (and probably needs to be used in 3) of the blocks, so maybe it would be more clear if it gets its own variable like multiple_arrays = self.num_arrays > 1 above the if model block.

else:
inv_fun = inverter.pvwatts
return inv_fun(p_dc, self.inverter_parameters['pdc0'], **kwargs)
elif model == 'adr':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to handle the case that model == 'adr' and self.num_arrays > 1 with another ValueError.

@cwhanse
Copy link
Member Author

cwhanse commented Jan 26, 2021

With PVSystem.get_ac do we want to deprecate e.g. PVSystem.snlinverter? The separate inverter methods aren't used by get_ac.

Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new method looks good aside from trivial things. I do think we should deprecate the other methods in this PR.

pvlib.inverter.pvwatts_multi
"""
model = model.lower()
mulitple_arrays = self.num_arrays > 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mulitple --> multiple

@cwhanse
Copy link
Member Author

cwhanse commented Jan 26, 2021

I do think we should deprecate the other methods in this PR.

They can't be deprecated until after ModelChain.ac_model is reworked to use PVSystem.get_ac.

@cwhanse cwhanse added this to the 0.9.0 milestone Jan 26, 2021
Copy link
Member

@wholmgren wholmgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can work the ModelChain changes in a follow up PR.

vdcs = pd.Series(np.linspace(0, 50, 3))
idcs = pd.Series(np.linspace(0, 11, 3))
pdcs = idcs * vdcs
pacs = system.get_ac('sandia', vdcs, pdcs)
Copy link
Member

@wholmgren wholmgren Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this test passed in the CI because vdcs and pdcs are swapped from their correct order. It failed when I ran the tests locally. Fixed in #1150

I think we had a similar issue with the CI in the last year. Maybe there's something wrong in our configuration.

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 this pull request may close these issues.

Consider replacing inverter methods in PVSystem with a single method with a kwarg
2 participants