Skip to content

irradiance.globalinplane appears redundant with total_irrad #422

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 Feb 12, 2018 · 5 comments
Closed

irradiance.globalinplane appears redundant with total_irrad #422

cwhanse opened this issue Feb 12, 2018 · 5 comments
Labels
Milestone

Comments

@cwhanse
Copy link
Member

cwhanse commented Feb 12, 2018

Suggest removing globalinplane.

@wholmgren
Copy link
Member

I think they're different, but I'm struggling to think of a good use case for globalinplane.

globalinplane computes POA global from modeling intermediates (AOI, DNI, POA diffuse).

total_irrad computes POA components from more basic information (system specs, solar position, DNI, GHI, DHI).

@wholmgren wholmgren added the api label Feb 12, 2018
@jforbess
Copy link
Contributor

jforbess commented Feb 13, 2018 via email

@wholmgren
Copy link
Member

total_irrad accepts scalar (fixed) or vector (tracking) surface azimuth and tilt. It does not automatically compute tracking angles, though.

My post above was a bit misleading. I suggested that globalinplane returns only POA global, but it actually returns POA global, POA direct, and POA diffuse (total). We could probably come up with a better name for globalinplane. Something like poa_components.

Thinking through this a little more... total_irrad does 3 things:

First, total_irrad provides a unified interface to the various diffuse sky models. This aspect could be refactored into a separate function (get_diffuse?) and used within total_irrad.

Second, total_irrad calls the grounddiffuse function.

Third, total_irrad assembles the POA components into a data structure (DataFrame or OrderedDict). total_irrad could be refactored to use a renamed globalinplane internally. The renamed globalinplane would need to include the sky diffuse and ground diffuse to its output for API consistency.

So, total_irrad would be reduced to a wrapper for the 3 steps described above. It would not do any calculations itself.

While we're at it, I'd welcome suggestions for a better name than total_irrad.

@cwhanse
Copy link
Member Author

cwhanse commented Feb 13, 2018

I think those ideas are good, reduce total_irrad to a wrapper, and refactor the wrapped functions. My instinctive reaction to 'total_irrad' is 'get_irradiance_components'

I like function names that start with verbs.

@cwhanse
Copy link
Member Author

cwhanse commented May 7, 2018

Closed via #427

@cwhanse cwhanse closed this as completed May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants