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

Updating Townsends snow model references #2384

Merged
merged 8 commits into from
Feb 12, 2025
Merged

Conversation

ayushjariyal
Copy link
Contributor

@ayushjariyal ayushjariyal requested a review from cwhanse February 9, 2025 17:51
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.

@ayushjariyal you will need to fix the line length and indentation in reference [2]

Please add a note to the whatsnew file for v0.11.3

@ayushjariyal ayushjariyal requested a review from cwhanse February 10, 2025 03:12
@ayushjariyal ayushjariyal requested a review from cwhanse February 10, 2025 09:18
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.

You can add a line after this one in the whatsnew file

* Manoj K S (:ghuser:`manojks1999`)

@ayushjariyal ayushjariyal requested a review from cwhanse February 10, 2025 17:18
@cwhanse
Copy link
Member

cwhanse commented Feb 10, 2025

@ayushjariyal can you fix the line length issues in your editor? I'm having difficulty suggesting corrections here.

@ayushjariyal
Copy link
Contributor Author

@cwhanse I’m having trouble understanding why the tests are failing.

Screenshot from 2025-02-10 23-16-15

Could you please help clarify the issue?

@cwhanse cwhanse added this to the v0.11.3 milestone Feb 10, 2025
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.

Thanks @ayushjariyal

@cwhanse
Copy link
Member

cwhanse commented Feb 10, 2025

@cwhanse I’m having trouble understanding why the tests are failing.
Could you please help clarify the issue?

You can ignore those failures. They are unrelated to the changes you made and they are happening in other PRs.

@ayushjariyal
Copy link
Contributor Author

Thank you for the clarification!

Copy link
Contributor

@IoannisSifnaios IoannisSifnaios left a comment

Choose a reason for hiding this comment

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

LGTM! Only one suggestion from my side.

pvlib/snow.py Outdated
Comment on lines 295 to 296
.. [3] Townsend, T. (2025). Snow Events Definition.
:doi:`10.13140/RG.2.2.14299.68647`
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we use the actual title of the poster and also mention that it was presented in SPI conference in 2013 (see info in the image below)

image

Copy link
Member

Choose a reason for hiding this comment

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

I agree - I overlooked the "SPI 2013" conference label. Reference text should be

[3] Townsend, T. (2013). Predicting PV Energy Loss Caused by Snow. Solar Power International, Chicago IL. :doi:10.13140/RG.2.2.14299.68647

@ayushjariyal ayushjariyal requested a review from cwhanse February 12, 2025 05:19
pvlib/snow.py Outdated
Available at: https://www.nrel.gov/docs/fy25osti/90585.pdf
.. [3] Townsend, T. (2013). Predicting PV Energy Loss Caused by Snow.
Solar Power International, Chicago IL.
doi:`10.13140/RG.2.2.14299.68647`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
doi:`10.13140/RG.2.2.14299.68647`
:doi:`10.13140/RG.2.2.14299.68647`

One last edit I think.

@cwhanse
Copy link
Member

cwhanse commented Feb 12, 2025

Thanks @ayushjariyal !

@cwhanse cwhanse merged commit 1a2a2be into pvlib:main Feb 12, 2025
21 checks passed
@ayushjariyal
Copy link
Contributor Author

@cwhanse Thank you for reviewing and merging my PR! I really appreciate the support. Also, does this organization participate in GSoC? I'm excited to contribute further!

@cwhanse
Copy link
Member

cwhanse commented Feb 12, 2025

@ayushjariyal we do participate in GSoC, the current list of project ideas is here.

echedey-ls pushed a commit to echedey-ls/pvlib-python that referenced this pull request Feb 19, 2025
* Updating  Townsends snow model references

* Applying reviews

* Adding a note to the whatsnew file for v0.11.3

* Applying review

* Add myself as contributor and apply reviews

* Fix the line length issue

* Adding actual title of the poster

* correct doi syntax
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.

loss_townsend: Update Townsend Snow Model References
3 participants