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

Townsend snow (building on #1251) #1468

Merged
merged 29 commits into from
Sep 12, 2022
Merged

Conversation

reepoi
Copy link
Contributor

@reepoi reepoi commented Jun 7, 2022

This builds off of pull request #1251 to add the changes suggested in the conversation there and wrap it up.

  • Closes Implement monthly snow loss model #1246
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries in docs/sphinx/source/reference for API changes.
  • Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

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.

Thanks @reepoi this is looking pretty good! A few comments below.

@kandersolar kandersolar added this to the 0.9.2 milestone Jun 12, 2022
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

Editorial comments, thanks @reepoi

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.

One or two things below that should be addressed before merging.

A comment more about the model than this PR: the geometry of the ground interference term seem strange to me in that it's only accounting for half of what I would expect from a snow pile. I expected the pile to be symmetrical (angle of repose on both sides), but I can only get the math to match the reference if one side of the pile is vertical. Is the idea that the forward momentum of the shedded snow allows it to populate only the "forward" half of the pile? To be explicit using the labels in my diagram below, I expected the dissipation area to comprise A_1, A_2, and their reflected counterparts on the other side of the "drop height" line, but it seems that Townsend & Powers don't count the reflected counterparts.

image

@cwhanse
Copy link
Member

cwhanse commented Jul 20, 2022

A communication with the model author T. Townsend: "the pile of snow was modeled as a triangle atop a rectangle, with yes, a vertical wall on the "north" side of the pile. This assumption, while an engineering approximation, is based on observing the shape of shedded snow piles, piles that are shaped by the slight forward (i.e., southward) velocity of the sliding/falling snow and ice."

@cwhanse cwhanse mentioned this pull request Jul 26, 2022
9 tasks
@kandersolar kandersolar modified the milestones: 0.9.2, 0.9.3 Aug 19, 2022
@kandersolar kandersolar mentioned this pull request Sep 8, 2022
Copy link
Member

@cwhanse cwhanse left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Thanks @abhisheksparikh for getting this started and @reepoi for finishing it! Great to see this merged.

@kandersolar kandersolar merged commit 875aa10 into pvlib:master Sep 12, 2022
@kandersolar kandersolar mentioned this pull request Sep 12, 2022
8 tasks
@cwhanse cwhanse mentioned this pull request Sep 13, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement monthly snow loss model
4 participants