Skip to content
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

ValueError on clear-sky models page in Users Guide #1914

Closed
kandersolar opened this issue Nov 15, 2023 · 2 comments · Fixed by #1915
Closed

ValueError on clear-sky models page in Users Guide #1914

kandersolar opened this issue Nov 15, 2023 · 2 comments · Fixed by #1915

Comments

@kandersolar
Copy link
Member

The clear-sky docs page has an issue:

In [156]: clear_samples = clearsky.detect_clearsky(ghi, cs['ghi'], cs.index, 10)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-156-a86cb0f5cb3e> in <module>
----> 1 clear_samples = clearsky.detect_clearsky(ghi, cs['ghi'], cs.index, 10)

~/checkouts/readthedocs.org/user_builds/pvlib-python/checkouts/stable/pvlib/clearsky.py in detect_clearsky(measured, clearsky, times, infer_limits, window_length, mean_diff, max_diff, lower_line_length, upper_line_length, var_diff, slope_dev, max_iterations, return_components)
    823     if len(times) < samples_per_window:
    824         raise ValueError(f"times has only {len(times)} entries, but it must \
--> 825                            have at least {samples_per_window} entries")
    826 
    827     # generate matrix of integers for creating windows with indexing

ValueError: times has only 30 entries, but it must                            have at least 50 entries

Presumably the problem is that this code is passing the value 10 by position to the new infer_limits parameter added in #1784 instead of to window_length like it used to. I expect it would be fixed by editing that line in clearsky.rst to specify the parameter by name: window_length=10.

@matsuobasho
Copy link
Contributor

matsuobasho commented Nov 20, 2023

I'd like to tackle this issue.
Seems like your fix suggestion worked. Output after running make html:

image

I have a question about PR's since this is only my second issue. What is the procedure for updating my fork of the repo as I move from issue to issue? Should I delete each such forked repo and create a new fork for every new issue (a little inefficient). Right now my fork is actually ahead of main by 10 commits, I think because a PR gets squashed into 1 commit. If I just start building atop of this, this will most likely cause a problem since those commits have already been incorporated.

Thanks.

@cwhanse
Copy link
Member

cwhanse commented Nov 20, 2023

Should I delete each such forked repo and create a new fork for every new issue

I update my main branch, then create a new branch for each PR. I don't think deleting your fork and creating a new one is needed.

If you don't make new branches, then the new PR may contain commits in other PRs that haven't yet been merged.

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

Successfully merging a pull request may close this issue.

3 participants