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

System module sphinx documentation #146

Merged
merged 34 commits into from
Dec 12, 2022

Conversation

kperrynrel
Copy link
Member

@kperrynrel kperrynrel commented Jul 7, 2022

Description

Thank you for your contribution! Please provide a brief description of the problem and the proposed solution or new feature (if not already fully described in a linked issue)

Checklist

The following items must be addressed before the code can be merged.
Please don't hesitate to ask for help if you are unsure of how to accomplish any of the items.
You are free to remove any checklist items that do not apply or add additional items that are
not on this list

  • Closes #xxx
  • Added new API functions to docs/api.rst
  • Clearly documented all new API functions with PEP257 and numpydoc compliant docstrings
  • Adds description and name entries in the appropriate "what's new" file
    in docs/whatsnew
    for all changes. Includes link to the GitHub Issue with :issue:`num`
    or this Pull Request with :pull:`num`. Includes contributor name
    and/or GitHub username (link with :ghuser:`user`).
  • Non-API functions clearly documented with docstrings or comments as necessary
  • Added tests to cover all new or modified code
  • Pull request is nearly complete and ready for detailed review

@kperrynrel kperrynrel added the documentation Improvements or additions to documentation label Jul 12, 2022
@kperrynrel kperrynrel linked an issue Jul 12, 2022 that may be closed by this pull request
8 tasks
@kperrynrel kperrynrel added this to the v0.1.2 milestone Jul 12, 2022
@kperrynrel kperrynrel removed a link to an issue Jul 12, 2022
8 tasks
Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing offline discussion with @kperrynrel: at least in the PVWatts fits, the inaccuracy stems from several sources:

  1. Assuming static ambient temperature of 25 degrees
  2. Not-great accuracy of the Ineichen model w/ fixed turbidities
  3. Assuming isotropic diffuse irradiance in the transposition, and perhaps other minor parts of the performance model

We came up with an alternative approach using PSM3 data that addresses (1) and (2). Let's take up (3) separately in #147.

Other notes from a brief high-level look:

  • Do we need both of the CSV files? Seems like only one of them gets used in the examples.
  • The CSVs are pretty big at 2.3 MB each. The (compressed) dist files for v0.1.1 are only 85 kB so including these is a big jump. At least for the purposes of this example, I suspect there's not much value in having multiple years of data since a single year (or maybe even just 6 months) has all the relevant seasonality being fitted here. Should we pare those files down?
  • I think the docs/examples/.pylint.d/infer-orientation-daily-peak1.stats file can/should be removed

docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
@kandersolar kandersolar mentioned this pull request Jul 26, 2022
8 tasks
kperrynrel and others added 2 commits July 26, 2022 11:07
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
Co-authored-by: Kevin Anderson <kevin.anderson@nrel.gov>
@kperrynrel kperrynrel mentioned this pull request Jul 26, 2022
3 tasks
@kperrynrel
Copy link
Member Author

@kanderso-nrel I made the recommended updates. Let me know when you're good to move ahead on this, and I'll send it over to Cliff!

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some ongoing offline discussion about whether the daily-peak function currently in pvanalytics actually implements the desired functionality -- more on that in the coming days.

Anyway a few comments below about the other two examples, otherwise LGTM! Not hitting "approve" yet since we're still undecided on the daily-peak point.

docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-fit-pvwatts.py Outdated Show resolved Hide resolved
docs/examples/system-tracking.py Outdated Show resolved Hide resolved
docs/examples/infer-orientation-daily-peak.py Outdated Show resolved Hide resolved
@kperrynrel
Copy link
Member Author

@kanderso-nrel I'm for us delaying this PR until after PVPMC, as we are actively working on these functions in our new data validation. Maybe we just push through the orientation PR before August 22nd?

@kperrynrel kperrynrel removed this from the v0.1.2 milestone Aug 16, 2022
@kperrynrel kperrynrel added this to the v0.1.3 milestone Nov 17, 2022
@kandersolar kandersolar mentioned this pull request Dec 9, 2022
@kperrynrel
Copy link
Member Author

@kanderso-nrel I cleaned things up for a new release. Let me know if this looks solid and I'll add Cliff as a reviewer. Thanks!

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK with me

docs/whatsnew/0.1.2.rst Show resolved Hide resolved
@kperrynrel kperrynrel requested a review from cwhanse December 12, 2022 16:41
Co-authored-by: Cliff Hansen <cwhanse@sandia.gov>
@kperrynrel kperrynrel merged commit b212fff into pvlib:main Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants