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.

Sign up for GitHub

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

Jump to bottom

Verification of forecasts and satellite data - Summaries dialog #5847

Merged
merged 37 commits into from
Jul 9, 2020
Merged

Verification of forecasts and satellite data - Summaries dialog #5847

merged 37 commits into from
Jul 9, 2020

Conversation

shadrackkibet
Copy link
Collaborator

Fixes #5843. This is still work in progress. Continuous options working now. Coming soon!

@shadrackkibet
adding continuous verification functions
a5ab5a5
@shadrackkibet
adding controls
074560c
@shadrackkibet
pre-functionality implementation
2e1d149
@shadrackkibet
bug fix with calculation of summaries
bb87202
@shadrackkibet
adding missing values option and printing in the output window
b8d7255
@shadrackkibet
implementation of verification options in the subdialog
0784b1b
@shadrackkibet
user control implementation: verification and hydroGOF options
448ae09
@shadrackkibet
removed overllaping functions from verification
ec8af81
@shadrackkibet
adding binary and categorical measures
ca75c44
@shadrackkibet
designer adjustments
61acced
@shadrackkibet
removed tooltips and unnecessary function declarations
3817802
@shadrackkibet
binary and categorical measures implementation
1aaaa1b
@shadrackkibet
designer adjustments
8b349f8
@shadrackkibet
enabling and disabling appropriate tab in the subdialog. showing requ…
43165e0 
…ired data types in the selector when on a particular receiver
@shadrackkibet
setting correlation as default summary checked
f9ba531
@shadrackkibet
checking if the summary list is empty and notifying the user that OK …
67b6bec 
…will not be enabled
@shadrackkibet
Copy link
Collaborator Author

@rdstern this is now working. Please do some initial testing?

@rdstern
Copy link
Collaborator

rdstern commented Jun 23, 2020

Great - will do!

@shadrackkibet
Copy link
Collaborator Author

@dannyparsons any comments on the structure of the dialog? Here are the screenshots.

image

image

image
image

@shadrackkibet
adding check to avoid checked an unchecking checkboxes in the wanted …
b5c12db 
…state also renamed list of continuous checkboxes
@shadrackkibet shadrackkibet mentioned this pull request Jun 24, 2020
Merged
rdstern
rdstern requested changes Jun 24, 2020
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

I comment here on the new menu structure. The suggested change has been slightly misunderstood.
I was suggesting Compare: Calculation and Compare: Summary in their own little section (i.e. with a line above and below of the Climatic > Prepare menu.) Not that they go down to a third level.

@shadrackkibet
Copy link
Collaborator Author

Thanks for the clarification. I will do this change in another open pull request which contain frmmain changes.

@rdstern
Copy link
Collaborator

rdstern commented Jun 24, 2020

This looks great. On the main dialogue could you change the 2 receivers to items that are more general. I showed it to Danny and he suggested Observed: and Estimated:.

@shadrackkibet
Copy link
Collaborator Author

@rdstern that is now sorted out.

rdstern
rdstern requested changes Jun 26, 2020
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

I assumed the Climatic > Prepare > Compare: Summary could be checked. I don't have a problem that it is in the old menu structure, but the Summary option isn't disabled

@shadrackkibet
Copy link
Collaborator Author

I am going to update this branch with the master then you will be able to see it in the new position.

@Patowhiz Patowhiz mentioned this pull request Jun 30, 2020
Merged
@lloyddewit
Copy link
Contributor

@Patowhiz thank you for the peer review. Are all your comments resolved? May @rdstern and I proceed with final test and review? thanks

@Patowhiz
Copy link
Contributor

Patowhiz commented Jul 7, 2020

@shadrackkibet Thank you for the responses. @lloyddewit I'm satisfied with the changes you can take over.

rdstern
rdstern previously approved these changes Jul 8, 2020
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

Happy to approve now. This seems to be a good example where the 2 types of checking is working. Over to @lloyddewit for the possible second approval.

lloyddewit
lloyddewit requested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

@shadrackkibet @Patowhiz This looks good. Thanks also for the thorough peer review. I found your discussions interesting.
@shadrackkibet I have some small suggestions. If you can resolve these then I can approve and merge, thanks

instat/ucrContinuousVerification.vb Outdated Show resolved Hide resolved
instat/dlgCompareSummary.vb Show resolved Hide resolved
instat/dlgCompareSummary.vb Show resolved Hide resolved
instat/dlgCompareSummary.vb Outdated Show resolved Hide resolved
instat/dlgCompareSummary.vb Outdated Show resolved Hide resolved
instat/static/InstatObject/R/Backend_Components/summary_functions.R Outdated Show resolved Hide resolved
instat/ucrContinuousVerification.vb Outdated Show resolved Hide resolved
instat/ucrContinuousVerification.vb Outdated Show resolved Hide resolved
instat/ucrContinuousVerification.vb Outdated Show resolved Hide resolved
instat/ucrContinuousVerification.vb Outdated Show resolved Hide resolved
@lloyddewit
Copy link
Contributor

@rdstern You already approved this before. The changes triggered by the peer review should not have affected the software logic so the risk of regression is small. Please could you approve again so we can merge? thanks

rdstern
rdstern approved these changes Jul 9, 2020
View reviewed changes
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

Happy to approve again

@lloyddewit lloyddewit merged commit 0fb7054 into IDEMSInternational:master Jul 9, 2020
@shadrackkibet shadrackkibet deleted the compare branch August 3, 2020 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Patowhiz Patowhiz Patowhiz left review comments

@lloyddewit lloyddewit lloyddewit approved these changes

@rdstern rdstern rdstern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New dialogue called Verification Summary
4 participants
@shadrackkibet @rdstern @lloyddewit @Patowhiz