Added Degree button to the Climate>Prepare>Transform dialog

[anastasia-mbithe]

This fixes #6990.
Hello, @rdstern I have added the controls and functions. Though, this is still in progress. There are some small fixes I am still working on.

[rdstern]

@anastasia-mbithe I know this is still "work in progress". It is looking good already!
a) Could you add ther s, so the button at the top is called Degrees.
b) In the QC dialogue we have Tmax before Tmin. Could that be the order here too.
c) Changing the radio button options you have added, seems also to refresh the data selector. Does that need to be done?
d) And an extra bit of work. I am adding another issue (number #7069) on the same dialogue. I hope it is sufficiently small that you could include the changes here.

[anastasia-mbithe]

@rdstern Kindly have a look at this.

[maxwellfundi]

@rdstern this is ready for a re-review

[rdstern]

@anastasia-mbithe it looks like nice code and I assume is almost ready.
But I tried with dodoma and then with the tidy version from Ghana. I used max and min. It gives an error repeatedly saying argument 1 (or parameter 1 is empty. That was for diurnal range and also HDD. It seems to work ok for tmean.

[anastasia-mbithe]

@rdstern This is ready for review now. I sorted out the issue and added functions to the HDD, GDD and ModifiedGDD to work well when we have Tmin and Tmax.

[rdstern]

@anastasia-mbithe this is all looking pretty good.
a) Set yourself a high standard in the lining up of the controls you add. So here the tmax and tmin (and tmean) could be aligned with the controls above. And the labels could be in exactly the same vertical position.
b) In the variable names we produce I suggest they be in lower case. Keep the capitals (as you have them) on the dialogue. But make the resulting variable names as hdd and gdd, etc.
c) You may wish to check with wikipedia, but I think the hdd are positive values. So add a minus in the formula you produce.
d) In calculating gdd I got some very odd values. Then I realised it was because I had changed the baseline for hdd to 35, but kept the gdd as 15. It seemed to pick up the 35 in the logical part of the gdd formula. See below:

# Code generated by the dialog, Transform
min_temperature <- data_book$get_columns_from_data(data_name="ghana", col_names="min_temperature")
max_temperature <- data_book$get_columns_from_data(data_name="ghana", col_names="max_temperature")
group_by_station <- instat_calculation$new(type="by", calculated_from=list("ghana"="station"))
transform_calculation <- instat_calculation$new(type="calculation", function_exp="(((min_temperature + max_temperature) / 2) - 15) * (((min_temperature + max_temperature) / 2) > 35)", result_name="GDD", manipulations=list(group_by_station), save=2, before=FALSE, calculated_from=list("ghana"="max_temperature","ghana"="min_temperature"))
data_book$run_instat_calculation(calc=transform_calculation, display=FALSE)

d) The formula for the modified gdd is also wrong - and still has my 35 from the previous HDD.

# Code generated by the dialog, Transform
min_temperature <- data_book$get_columns_from_data(data_name="ghana", col_names="min_temperature")
max_temperature <- data_book$get_columns_from_data(data_name="ghana", col_names="max_temperature")
group_by_station <- instat_calculation$new(type="by", calculated_from=list("ghana"="station"))
transform_calculation <- instat_calculation$new(type="calculation", function_exp="(min(30, (min_temperature + max_temperature) / 2) - 35) * ((min_temperature + max_temperature) / 2) > 35", result_name="ModifiedGDD", manipulations=list(group_by_station), save=2, before=FALSE, calculated_from=list("ghana"="max_temperature","ghana"="min_temperature"))
data_book$run_instat_calculation(calc=transform_calculation, display=FALSE)

e) Make that name mgdd.

f) I wonder what the programmers will say about your repeated use of (min_temperature + max_temperature) / 2) in these formulae, rather than calculating and using tmean?

[anastasia-mbithe]

@rdstern you can have a look at this now.
I have disabled HDD,GDD and Modified GDD, when having tmin and tmax so that they can be used with tmean only, to avoid repetition of (tmin+tmax) / 2
For HDD, it returns a positive value other than 0 when the tmean<15(baseline)

[rdstern]

@anastasia-mbithe no that is not what I was meaning! The idea is to have efficient code, but not to limit the users. So please check with @maxwellfundi or others how to improve your code, so you can allow the R calculations to be efficient and not repetitive.

For example I don't like it and am no good at programming, but could your the second line become:
tmean <- (data_book$get_columns_from_data(data_name="ghana", col_names="max_temperature")+min_temperature)/2
I would like simpler than this and it may not work?

[anastasia-mbithe]

@rdstern Kindly have a look at this.

[rdstern]

The first 2 options seem to work. The up-down for the next 2 should display the point as they only go up by 0.1 at a time. And the third one isn't the same. Is there a reason for the difference. I changed the HDD to 35. I get an error for each of these. How much are you checking them, before asking me?

[anastasia-mbithe]

@rdstern Hello, is it okay if I just set the baseline to one decimal place since the increment is 0.1?

[rdstern]

@anastasia-mbithe you've got it! The 3 controls should all be to one decimal place. This is the way you increment. Also the actual data are usually recorded to one decimal. In addition, when you test, try with the up-down feature. But also confirm that typing an accepted number is also possible. When I want to change 15 to 35 it takes forever with the up-down. But I usually just type 35 and it works fine. That option should still remain.

[anastasia-mbithe]

@rdstern I have just added the one decimal place feature for the baseline.
From my end, am not getting any errors while using HDD, GDD and MGDD as you mentioned on Friday. I have been testing it using 15, 25, 35 as well as any random number I just type and am getting no errors. It's working well from my end.

[rdstern]

Please can you also try my example. I am using the tidy Ghana data from the library. And it still says:

That is on the third option, (HDD) after running the first 2 options, which seem to work fine. I tried again after a reset and it crashed me out.

I checked the up-down controls and they seem better now. But I wonder why you didn't change the limit control. There you just type anything you like. I tried typing aa and it accepted that. I didn't try running it. That should become another up-down.

Aha, I have now also tried with dodoma and it works. So it may be ok with a single station. But it crashes with multiple stations. You might like to examine the calculation code carefully, because this isn't a set of calculations that should need to worry about that aspect. All is just within a single row. So single or multiple stations should have no effect. Have you understood the calculations you are doing really carefully. If not, (and it is quite tricky stuff) then check with Haward or one of the others.

[anastasia-mbithe]

Hello @rdstern, That issue is fixed now and the dialog is working well with both single and multiple station data. I'll be glad if you check it out.

[rdstern]

@anastasia-mbithe you are right, it does now seem to be working well. @shadrackkibet or @lloyddewit good to check the code, so we can merge

[lloyddewit]

@anastasia-mbithe Thank you, this looks good.
I just had a few small questions.

[anastasia-mbithe]

@lloyddewit Thanks for the review, kindly check if the code is okay as you intended.

[lloyddewit]

thank you for the changes. I reviewed them all and I'm happy.
I unresolved one of the old comments, please could you check it?
I also have a couple of extra comments about readability.
Thanks

[anastasia-mbithe]

@lloyddewit I have implemented the suggested changes. For the unresolved old comment, I had replied in the conversation for the particular comment.

[rdstern]

still looks ok to me

[lloyddewit]

@rdstern another (but hopefully final) test/approval from you is needed, then it's ready for merge.
Would you like to merge this PR in 0.7.4 or is it safer to wait until next week?
thanks

[rdstern]

This is one I am happy to approve and would very much like to see in 0.7.4 (next Monday).
By the way I note one really nice new feature in the current grid. Usually, with temperaure data the first (1000) rows are empty. But now you can press the double arrow to go straight to the bottom of the grid. Very nice.

[lloyddewit]

@anastasia-mbithe
I think this is the fifth significant PR you've merged since September.
In these PRs, I found your code carefully written, consistent and tidy. This is important for readability and maintainability.
Congratulations on another successful merge and thank you for your contributions.

A small point: In your opening comment, I changed 'This fixes issue #6990' to 'This fixes #6990'. I did this so that issue #6990 is closed when this PR is merged. If you specifically didn't want issue #6990 to be closed, then please let me know. Thanks

(A while ago, I may have incorrectly advised you or others to write 'fixes issue #nnnn' but GitHub doesn't recognise this.)

[anastasia-mbithe]

@lloyddewit Thank you so much for that and also for the correction, I appreciate.