Fit extremes: Added options to fit an extreme value distribution (EVD) to Data

[Ivanluv]

@africanmathsinitiative/developers this fixes #5887 and it from PR #5900

[Ivanluv]

@shadrackkibet you can now review this

[lloyddewit]

@Ivanluv There are still some open peer review comments above. Please could you clarify whether they're already resolved, or if you still plan to accept/reject them? thanks

[Ivanluv]

@shadrackkibet could you review this again

[shadrackkibet]

• Looks better. Can you try to improve the design, alignments, naming etc? Otherwise, @lilyclements might find some time to help. See below.
  
  
  

[Patowhiz]

@Ivanluv would also be helpful if comments you could add comments explaining certain parts of code that are not clear in first glance.

[lloyddewit]

@Ivanluv does the commit above resolve all the peer review comments above, or are you still working on this? thanks for clarifying

[Ivanluv]

@lloyddewit this resolves all the peer reviews

[lloyddewit]

@Ivanluv I went through the previous peer review comments. Most of them seemed to be resolved, so I marked them as resolved. However, some still seem to be open. Please could you review all the comments above that are not marked as resolved?
If you reply to each comment (either accepting suggestion or giving reason for rejecting) then that would be very helpful, thanks

[Ivanluv]

@lloyddewit could you look at this again

[lloyddewit]

@Ivanluv I think there is one comment in instat/dlgExtremes.vb that is still open (see above). Please can you clarify? thanks

[Ivanluv]

@rdstern I have updated the branch

[rdstern]

Here is the code for one example now:
extRemes::fevd(x = stats::na.exclude(object = max), location.fun = ~station +
year, type = "GEV", method = "MLE")
I'm not sure this is a good way to try to cope with the missing values, so perhaps that should be undone. It still gives the same odd message when there are missing values in the data. I notice now that it also can't cope with missing values in the x'variables. We might just have to accept that for now.
A minor problem is that when the code, as in the example above, is on 2 lines, then it over-types in the title of the plot - at least for a combined plot.
Perhaps a quick look by @shadrackkibet could help. Then we merge and improve later.

[Ivanluv]

@shadrack how can I solve the over typing problem

[shadrackkibet]

• The function fevd has the parameter na.action which is meant to deal with missing values. I think to avoid the error we need to exclude missing. Therefore you need to set na.action = na.omit or na.action = na.exclude . For example
  extRemes::fevd(x = max, na.action = na.exclude, location.fun = ~station + year, type = "GEV", method = "MLE")
• I can't find an easy way to deal with the over-typing issue.

[shadrackkibet]

@Ivanluv can we try to get this merged? anything remaining?

[Ivanluv]

@shadrackkibet everything is done except for the over-typing issue

[rdstern]

The overtyping in the figure isn't important. It could be merged now, or the problems with missing values could be investigated further. Perhaps merge and have this as an issue?

[shadrackkibet]

Also, reset the save control here.

[shadrackkibet]

Only a few comments and then this should be ready. Since this is an initial implementation it will be good to get it in. Further improvements can be done after further testing.

• When I check the checkbox (Explanatory model for location or scale) and then reopen the dialog the controls below disappear.
• The save control doesn't reset.
• Rename the receiver "Response Variable:" which is consistent with "Model>Fit Model>General".

[Ivanluv]

@shadrackkibet I have resolved the issues you raised. Could you look at this again

[shadrackkibet]

Looks better now. Over to @lloyddewit for the next steps.

[lloyddewit]

@Ivanluv this looks good, just some small comments, thanks

[lloyddewit]

@Ivanluv Thanks for all the changes, there is just one comment still open

[lloyddewit]

@rdstern Please could you test? thanks

[rdstern]

Long-term my only confusion is with missing values. But I see the comment for the fevd function is as follows: "function to be called to handle missing values. Generally, this should remain at he default (na.fail), and the user should take care to impute missing values in
an appropriate manner as it may have serious consequences on the results."

So I suggest it be merged and we continue to test.