Skip to content

irradiance.detect_clearsky: possibly redundant input 'times' #954

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

Open
cwhanse opened this issue Apr 17, 2020 · 5 comments
Open

irradiance.detect_clearsky: possibly redundant input 'times' #954

cwhanse opened this issue Apr 17, 2020 · 5 comments
Labels

Comments

@cwhanse
Copy link
Member

cwhanse commented Apr 17, 2020

irradiance.detect_clearsky expects measured and clear-sky GHI to be input as Series, and has an input times as a DatetimeIndex.

times : DatetimeIndex

What is the use case for having times as a separate input? Should the function instead use measured.index?

@CameronTStark CameronTStark added this to the 0.7.3 milestone Apr 17, 2020
@wholmgren
Copy link
Member

measured and clear sky are allowed to be arrays

@adriesse
Copy link
Member

The chances of having measured and clear sky without time indexes are pretty slim though, aren't they?

@wholmgren
Copy link
Member

I agree and support the API change.

@cwhanse cwhanse modified the milestones: 0.7.3, 0.8.0 May 19, 2020
@wholmgren wholmgren modified the milestones: 0.8.0, 0.9.0 Aug 28, 2020
@cwhanse cwhanse mentioned this issue May 5, 2021
24 tasks
@cwhanse
Copy link
Member Author

cwhanse commented May 7, 2021

#1074 changed times from an arg to a kwarg with default None. Both measured and clearsky arguments can still be array. I think it does no harm to keep times for compatibility with any legacy code.

Recommend closing this issue.

@wholmgren wholmgren removed this from the 0.9.0 milestone May 7, 2021
@wholmgren
Copy link
Member

Fine with me to keep it open in an effort to simplify API or to close in an effort to clean the issues board. Either way not going into 0.9, so I removed the milestone.

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

4 participants