Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement irradiance.complete_irradiance with component sum equations #1567
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
Implement irradiance.complete_irradiance with component sum equations #1567
Changes from 15 commits
83f0990
5980930
1b2e5ed
08a18cf
21599f6
fbf0ee9
34344e9
24771c0
533d63b
db45cae
1a554e8
73e9b55
789540f
06cb584
b0ae6f4
e776928
74c954d
bebf349
f35b8e0
98e531c
9255aa2
ed6e106
9cfde3b
7cc3485
aff38d4
e595465
78a180d
4fac56f
b9ec987
e07f516
7ce6fa6
881779e
a0d6b18
99205b4
51272d4
01736b9
3958012
f8dd098
725d859
65fde50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess this is an artifact of switching to
apparent_zenith
. Maybe we shouldn't actually make that change, or if we do then it needs to be mentioned in the changelog. Let's keep this as "to discuss" also.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On technical merit,
apparent_zenith
would be better. But the practical difference is small and not worth annoying someone trying to reproduce archived results. I could go either way: change toapparent_zenith
, or name the parametersolar_zenith
and give a generic description.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other input on this change? We have two votes neither opposed but neither in strong favor. If that's the roll call, I'd say keep it as
zenith
.