-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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) |
There was a problem hiding this comment.
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
pvlib/pvsystem.py
Outdated
""" | ||
model = model.lower() | ||
if model == 'sandia': | ||
if self.num_arrays > 1: |
There was a problem hiding this comment.
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': |
There was a problem hiding this comment.
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
.
With |
There was a problem hiding this 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/pvsystem.py
Outdated
pvlib.inverter.pvwatts_multi | ||
""" | ||
model = model.lower() | ||
mulitple_arrays = self.num_arrays > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mulitple --> multiple
They can't be deprecated until after |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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.
docs/sphinx/source/api.rst
for API changes.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`
).Anticipates deprecation of the separate inverter methods PVSystem.snlinverter, .adrinverter, .sandia_multi, .pvwatts, and .pvwatts_multi after
ModelChain.ac_model
is reworked.