Skip to content

Unresolved attribute used in "SingleAxisTracker" #337

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
uvchik opened this issue Jun 12, 2017 · 1 comment · Fixed by #356
Closed

Unresolved attribute used in "SingleAxisTracker" #337

uvchik opened this issue Jun 12, 2017 · 1 comment · Fixed by #356
Labels
Milestone

Comments

@uvchik
Copy link
Contributor

uvchik commented Jun 12, 2017

I do not use tracking but working on PR330 it seems to me that there is something wrong.

https://github.com/pvlib/pvlib-python/blob/master/pvlib/tracking.py#L113

try:
   solar_zenith = kwargs['solar_zenith']
except KeyError:
    solar_zenith = self.solar_zenith

First of all one can simply write: solar_zenith = kwargs.get('solar_zenith', self.solar_zenith').

But at the moment the solar_zenith is neither an attribute of the class nor of the parent class. Furthermore the solar_zenith (apparent_zenith) is not obligate, but I should be. So we should change it to something like this:

- def get_irradiance(self, dni, ghi, dhi,
-                    dni_extra=None, airmass=None, model='haydavies',
-                    **kwargs):
+ def get_irradiance(self, dni, ghi, dhi, solar_zenith, solar_azimuth,
+                    dni_extra=None, airmass=None, model='haydavies',
+                    **kwargs):
-    try:
-        solar_zenith = kwargs['solar_zenith']
-    except KeyError:
-        solar_zenith = self.solar_zenith

To me it seems to be some kind of relict because according to the docstrings the zenith parameter is obligate.

You will find the same issue for the azimuth angle.

I think it is not urgent, because most users will understand that they have to pass zenith and azimuth to get the total irradiation and I will not have the time to write the according tests.

Maybe somebody who works with tracking may want to bring that in line.

@wholmgren
Copy link
Member

This is closely related to #351. I think the function signature should include the surface tilt & azimuth and the solar zenith & azimuth angles, then we can get rid of the inappropriate attribute nonsense.

Yes, it is a relic from when I was trying to figure out the scope and API of these classes.

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