-
Notifications
You must be signed in to change notification settings - Fork 103
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 Filled in Column in End Of Season Dialog. #9214
base: master
Are you sure you want to change the base?
Added Filled in Column in End Of Season Dialog. #9214
Conversation
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.
@MeSophie this is looking great. There is still one improvement to make (you may not have included that element yet?) and one question for you and @lilyclements
a) The status is still not adjusted for missing values. I used dodoma and made row 838 (17 April 1937) as missing. Then the results on the end of the rains (14 April) are given and end of the season is correctly missing. The status variable should then be NA, but it is still TRUE.
b) The question, is when you give the filled in column. Unless @lilyclements would like it always, I suggest you only give it when some years are FALSE in the status. That's when there IS something to be filled in. If there is nothing to be added - so no years are FALSE, then you don't add the filled variable.
c) Just to confirm when you do the status correctly, then the Filled in is only for status ==FALSE. If status == NA, then the filled in is also NA.
@rdstern what parameters have you set here? I am unable to reproduce this my end at the moment.
Agreed. If the status is |
@lilyclements and @MeSophie I used dodoma - as usual. |
@rdstern perfect thank you. I can see the issue now. We did a similar change in @MeSophie can you amend the
|
@MeSophie please tell me when I can test again? |
@rdstern I made the change according to @lilyclements code. Moreover, there's this error that suddenly appeared on line two of the code. I don't know if @lilyclements can help to solve it. Even If we right-click to convert a variable to a factor manually, we also get the same error. |
@MeSophie yes I have recently got that error. I think it has just been fixed (a few minutes ago), so can you update your branch to use the latest merged version. Then that should now be ok, and maybe soves the other problem too? |
@rdstern I update the branch and it works fine now. You can Already test the change made on the end_season_status. |
@rdstern from the way the calculation system is set up, we create the filled column at the same time that we create the status column. I believe that the easiest way to handle this is to have a checkbox option to include a filled column (I suppose this way the user can then rename it too if they have multiple of them, like the day of year). What are your thoughts on the addition of the filled column being a checkbox? |
@MeSophie and @lilyclements very happy to have a checkbox. I suggest unchecked by default. |
@MeSophie I did my usual sequence, so end of rains up to end April, and then end season from end of rains till end April. Now get this essor. |
@lilyclements Could you please help me to investigate on this error. It is caused by line |
…nts-patch-1 Update calculations.R
Allowing checks and conversions if join type differs between two data frames
@rdstern the problem is now fixed. Could you please test the dialog again? |
@MeSophie You now give the status as NA, when it should be FALSE. here is my example: Here there were many missing end of season, because it didn't happen by 30 April. I added a missing value in the data in 1937, after the end of the rains. There the NA in the status is correct. (That's how we distinguish between the missing data and the censored data. Can you also add the checkbox to Include Filled Data? |
@rdstern where should the status be I cannot open R-Instat at the moment given the most recent development version (it crashes, and shuts itself). If you could share your entire R script then that would help me out as I would then be able to debug in R! |
@lilyclements and @MeSophie I thought we agreed, like the start of the rains, that FALSE is where there is no end of the season by the defined end date? |
@rdstern thanks - I was running the sequence incorrectly and so it was working for me. Very confused. But, I'm able to replicate the error now. I think Sophie just missed a message before saying it can be retested. @MeSophie as stated here, can you amend the
|
@lilyclements Sorry I can see the difference between the actual end_season_status and the one you send. It's the same code as the one you sent last week and it's already implemented. It's the one that @rdstern says doesn't work. Dialog: End of Rains/Season
|
@lilyclements Could you please also provide the code for this column (I assume it is a new column as end_season_filled )? Thank you. |
@MeSophie thanks. It wasn’t running the full end_season_status for me but if it’s working now then great. the filled column code is exactly what you’re currently running with your end_season_filled, this just needs to be attached to a checkbox now (and the instat_calculation$new updated) |
@rdstern I am trying to understand what I am misinterpreting so I can fix the code.
In the below example, I set our row 838 to NA
|
@MeSophie what does this new checkbox in the length dialog do? Thanks. |
@lilyclements and @MeSophie your a) b) and c) above are perfect. But, when you look at my output above 1936 should be FALSE as it is for you. You can see from my figure above (where there were no missing values, that 1936 was NA and there no instances of FALSE in my record, though there were years, without missing, where the season didn't finish by day 121. |
Good puzzle - finally solved! We need to convert
This means we have to So we want to run something like this:
But then, what if there hasn't been a linked data frame created yet? We can't really do an So perhaps I should write a function which: (a) checks existence of a link Thoughts?
|
@lilyclements The checkbox is for the Filled data. |
@MeSophie OK, and what does it do? Does it fill in the data - if so, how do we know the end date? Etc. |
@lilyclements I tested the new change with our dodoma data and it works well but when change to Ghana data I obtain these notifications but the code is still produce the result.
|
@lilyclements I think @rdstern is the best to answer. |
@MeSophie and @lilyclements so far great. It works perfectly - so far - for dodoma! Let me continue. There is now a nice file for Zambia with 5 stations. Let me try that. |
@MeSophie and @lilyclements the problem is probably an old one I think you fixed in the start of the rains? Here is the error message with the end of the season at dodoma. It isn't just a multiple stations problem, but is when there is a station variable in the command. So I added the station for dodoma and here it is. I hope this helps. |
@MeSophie I can see this error has in it I didn't mean for this to be read in yet - or to be used (yet). I would put it into a function myself, if we were happy with the suggestion I gave, and then you would just have to read in the Waiting for feedback on that until I create that function and go from there. |
@MeSophie I hope the above is sufficient for you to make the corrections today? I'm really keen for this and the length to be included in the version at the end of this week. |
@rdstern from my understanding of @lilyclements comment, the error is coming from the @lilyclements does this mean that I don't need anymore this line of code |
… the same class as that of the from data frame
@MeSophie, great point. Yes we wouldn't want to run that You just need to run before the EoR/EoS code:
Can you also change the bit after the EoR/EoS code from:
to:
Changing the "after" isn't as important, but, it would be nice for consistency. I hope this is as straight forward as changing your |
I'll get started straight away. Thank you @lilyclements. |
Adding function in linking to convert variables in a to data frame to be the same class as that of the from data frame
@MeSophie and @lilyclements I get this error message It then produces results, but the status is still NA when it should be FALSE. I am sorry it is giving so much trouble! |
@MeSophie and @lilyclements this is a big advance and I think is almost there now! I have been checking first with Dodoma, including an added station variable. Then with Zambia 5 stations, that is in the library. Dodoma first. I have never had an error message. Here are the results for Dodoma: Now just 2 small things to note. a) First, the station linking variable is numeric, i.e. value 1 - it is labelled dodoma in the daily data. Now the results for Zambia: a) It works - no error messages. Thanks. Am I wrong, in thinking you guys are really close now? |
Comments Linking Station Variable Type
@rdstern this is now fixed (hopefully!) End Season Filled - NAs
@MeSophie apologies, can you change the
Zambia Data@rdstern where can I find the Zambia data? I can only see Moorings in R-Instat. I think I know what the issue might be! At least, I've found an issue to fix (which I think is what you're seeing in line 80):
If we look at what's happening, we can see this is because we take the last instance of rainfall, and that happens to be an NA. |
@lilyclements in the latest version 0.8.0, which is now in the download website, it is added to Moorings in the climatic library. I'll look at the other question when I am off the bus! |
Your 838- 17 April is made missing. Then the end of the rains is correctly missing, because that may otherwise have been the last day up to 30 April. Any day mssing up to 30 April should make the end of the rains missing. And then status should also be NA. |
@rdstern The end_season_filled is now fixed. Please have a look. |
Here is the sitation now: By the way I made 838 and 1180 missing, and the end of the rains copes well. 838 is after the end of the rains in 1937 so it doesn't know whether the date it found is last. 1180, in 1938 is before the end of the rains, so it doesn't matter that it is missing, because there are 10mm after that date. Congragtulations both of you, on the code, because that's pretty clever. And one reason I suggest for a statistics package for summaries, rather than coding in a spreadsheet, is because they cope well with missing values. We need to write this all up and I like this example! I can't check with these events, whether the end of the season having a missing value is catered for, because the end date is ideantical for the rains and the season. I'll make it a bit later to check that too! So I have changed the end of the season last date to 5 May and also made day 122 -1 May missing in 1936. Here are the new results, and they are "interesting"! So they correctly have the end of the rains as ok, because the missing is in May. And they correctly have the end of the season as missing. So the code is coping brilliantly with the missing values in the day and the date variables. So our baseline is solid! Now notic 1936 the season status is now correctly missing! Yippee. It is incorrect in 1937, because it should be missing whenever the day is missing, because of a missing value. So 1936 and 1937, should both have the status as missing. And the filled value now. In 1937 when the status is incorrectly FALSE, (should be NA) the filled value is correctly NA. In 1936, when the status is correctly NA, the filled value is incorrectly 126. And the filled value, should of course, only be 126 when the status is FALSE. Otherwise it should be the end-season date! |
Problem 1:
To fix this, we need to amend the
Problem 2: The issue with the end season filled values is a really simple one! It's just that the parameters are being given in the wrong order, so R is taking our "yes" and "no" the wrong way in our "ifelse" statement, as we aren't giving the parameters. @MeSophie can you add in
Everything else there is looking great! As I say, I'll get to the other issue shortly (Monday?) but this one caught my eye in the meantime as something to fix. |
Fixes #9117
@rdstern @lilyclements I made some modification on End Rain/Season dialog. Please have a look.