Skip to content

Return sky diffuse and components as dataframe in irradiance.perez #434

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 22, 2018 · 5 comments · Fixed by #565
Closed

Return sky diffuse and components as dataframe in irradiance.perez #434

cwhanse opened this issue Feb 22, 2018 · 5 comments · Fixed by #565
Labels
Milestone

Comments

@cwhanse
Copy link
Member

cwhanse commented Feb 22, 2018

irradiance.perez can return isotropic, circumsolar and horizon components of sky diffuse irradiance if return_components = True. Currently, if return_components then results are returned as a tuple (sky_diffuse, diffuse_components) with the elements being a pandas.Series and pandas.Dataframe, respectively.

I propose that we always return a Dataframe with column sky_diffuse and optionally columns for isotropic, circumsolar, horizon. This avoids having the output of the perez model of a different type (tuple instead of pandas object) and having to unpack the tuple when diffuse_components are requested.

@wholmgren
Copy link
Member

This is an interesting idea. I'm concerned about maintaining API compatibility across all of the transposition models. I think the default behavior of all of the models should be the same, if possible. So, I'm reluctant to default to returning a one-column DataFrame from perez while the others return Series.

We could make similar changes to all of the models. I don't know if that would be useful or busy work. What would the keys look like for each function? For example

isotropic returns sky_diffuse, isotropic?
haydavies returns sky_diffuse, isotropic, circumsolar?
perez returns sky_diffuse, isotropic, circumsolar, horizon?

@cwhanse
Copy link
Member Author

cwhanse commented Feb 22, 2018

Good point about compatibility. We could return a Series if return_components = False rather than a one column data frame. The hiccup for me is dealing with the tuple instead of a pandas object.

We could propagate the component capability into the other sky diffuse models, although perez is likely to be the most frequently used for this purpose.

@wholmgren
Copy link
Member

Sure, that sounds like a quick fix then.

For reference, the return_components feature was added in #259. It does not appear that we discussed the idea of returning a single DataFrame with the total and the components.

@wholmgren wholmgren added the api label May 11, 2018
@wholmgren wholmgren added this to the 0.6.0 milestone May 11, 2018
@cwhanse cwhanse mentioned this issue Aug 27, 2018
@wholmgren
Copy link
Member

wholmgren commented Sep 6, 2018

@cwhanse I suggest that we limit the scope of this particular issue to irradiance.perez and create new issues if want to expand this feature to other transposition models. I'm not sold on the API over all which is why I'm reluctant to put it into more models unless it's really necessary.

@cwhanse
Copy link
Member Author

cwhanse commented Sep 6, 2018

@wholmgren I agree, irradiance.perez for this PR.

@wholmgren wholmgren changed the title Return sky diffuse irradiance components in sky diffuse dataframe Return sky diffuse and components as dataframe in irradiance.perez Sep 6, 2018
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 a pull request may close this issue.

2 participants