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

Added SOLRAD_behavior EstimateFromMean #345

Merged

Conversation

tefracky
Copy link
Contributor

Added a new possibility to calclate the solrad value. It is the mean value of the sun hours and the temperature and called EstimateFromMean. It shoult improve the accuracy of a calculated solrad value.

@tefracky tefracky marked this pull request as ready for review April 13, 2024 15:56
@jeroenterheerdt
Copy link
Owner

I love this, but the name is not good. Please add a better name instead of EstimateFromMean.

@tefracky
Copy link
Contributor Author

I am not a native English speaker, so do you have any suggestions?

@jeroenterheerdt
Copy link
Owner

neither am I, but since it's taking the average of the sunhours and the temperature, I'd propose something like EstimateFromSunHoursAndTemperature. Of course, update the descriptions in the language files as well.

For context, at first I was going to close this because it seemed to do exactly what EstimateFromTemp does, but only after looking at the code I realized that is actually not what it does. Imagine the confusion ti the user or future developer if we don't give things good names.

@tefracky
Copy link
Contributor Author

It's done. I also fixed some markdown formatting in the Readme (tailing whitepaces and missing blank lines).

@@ -24,6 +24,7 @@ class SOLRAD_behavior(Enum):
EstimateFromTemp="1"
EstimateFromSunHours="2"
DontEstimate="3"
EstimateFromMean="4"
Copy link
Owner

Choose a reason for hiding this comment

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

EstimateFromMean is not the right name. Let's give this a more descriptive name.

@@ -143,7 +143,8 @@
"translated-options": {
"DontEstimate": "Nicht berechnen",
"EstimateFromSunHours": "Basierend auf den Sonnenstunden",
"EstimateFromTemp": "Basierend auf der Temperatur"
"EstimateFromTemp": "Basierend auf der Temperatur",
"EstimateFromMean": "Basierend auf dem Mittelwert"
Copy link
Owner

Choose a reason for hiding this comment

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

EstimateFromMean is not the right name. Let's give this a more descriptive name.

@@ -143,7 +143,8 @@
"translated-options": {
"DontEstimate": "Do not estimate",
"EstimateFromSunHours": "Estimate from sun hours",
"EstimateFromTemp": "Estimate from temperature"
"EstimateFromTemp": "Estimate from temperature",
"EstimateFromMean": "Estimate from mean"
Copy link
Owner

Choose a reason for hiding this comment

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

EstimateFromMean is not the right name. Let's give this a more descriptive name.

@jeroenterheerdt jeroenterheerdt merged commit 5284c35 into jeroenterheerdt:master Apr 26, 2024
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants