ModelChain+SingleAxisTracker aoi calculations are wrong, too easy to miscalculate AOI using SingleAxisTracker

[wholmgren]

tl;dr

A combination of bad API choices led to an AOI calculation problem when using SingleAxisTracker objects with ModelChain. One possible solution requires minor changes to the SingleAxisTracker API and moving a few lines of code in ModelChain.

Background

The PVSystem initialization parameters surface_tilt and surface_azimuth are optional with default values of 0. Arguably, these should be required parameters for a PVSystem object, but that's not such a big deal. The problem comes with the SingleAxisTracker class. Because SingleAxisTracker inherits from PVSystem, and because we don't do anything special to prevent this, SingleAxisTracker objects are also initialized with self.surface_tilt = 0 and self.surface_azimuth = 0. These SingleAxisTracker attributes should not exist or they should be None or nan.

The next problem occurs in the SingleAxisTracker.get_aoi method, which is simply inherited from PVSystem.get_aoi. This get_aoi method relies on the self.surface_azimuth and self.surface_tilt parameters. That's a problem for SingleAxisTracker since we generally don't know these parameters at system construction time. A user could get the right answer by calculating the system's tracking data and then setting the angle attributes to the tilt and azimuth results, but that seems like a bad design and out of character with the rest of the library. It's especially problematic for SingleAxisTracker because its objects are initialized with tilt and azimuth values (0) that yield AOI numbers that might look reasonable to some at first glance. A better initialization method would have ensured that the SingleAxisTracker.get_aoi method failed or returned nans.

The ModelChain.prepare_inputs method calculates AOI to support the AOI losses model (which has its own issues, unfortunately: #339). These lines are wrong for SingleAxisTracker system objects.

Solution

I think the solution is to:

1. Implement a SingleAxisTracker.get_aoi method that requires surface_tilt and surface_azimuth parameters.
2. Move the ModelChain AOI calculation inside the if isinstance(self.system, SingleAxisTracker) block.

This should be resolved in conjunction with #337.

[cwhanse]

Maybe a variant on Solution #2: we could have get_aoi check the system type to determine the calculation technique. That would maintain the simplicity of the get_aoi signature.