search

PhilBoileau / cvCovEst

An R package for assumption-lean covariance matrix estimation in high dimensions
https://philboileau.github.io/cvCovEst/
Other
13 stars 4 forks source link

Plot labs #42

Closed PhilBoileau closed 3 years ago

PhilBoileau commented 3 years ago

Replacing "empirical risk" by "cross-validated risk" or "CV risk" in plot labels, column names, and documentation.

PhilBoileau commented 3 years ago

Great, thanks for reviewing and updating the eigenvalue plot legend. The only (minor) remaining issue was that the column names in the 5-number summary tables of the cross-validated risk distributions are in lower case, and one or two have underscores instead of spaces. I'd prefer names that a more "publication ready". I tried changing this directly in empRiskByClass(), but it caused issues downstream. Any suggestions?

bcollica commented 3 years ago

Ah, okay. Let me take a closer look this afternoon, I’m sure it’s a quick fix!

Brian

Sent from my iPhone

On Feb 21, 2021, at 8:29 AM, Phil Boileau notifications@github.com wrote:

 Great, thanks for reviewing and updating the eigenvalue plot legend. The only (minor) remaining issue was that the column names in the 5-number summary tables of the cross-validated risk distributions are in lower case, and one or two have underscores instead of spaces. I'd prefer names that a more "publication ready". I tried changing this directly in empRiskByClass(), but it caused issues downstream. Any suggestions?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or unsubscribe.

PhilBoileau commented 3 years ago

Sounds good, thanks. There's no rush!

bcollica commented 3 years ago

Hey Phil, I think the labels should be good to go now. Also, if you like, I can change empRiskByClass to be cvRiskByClass. It's minor, but it would be more consistent. Or maybe it's best to hold off until another version update, seeing as how we just got listed on CRAN? Whatever you think makes sense.

Brian

On Sun, Feb 21, 2021 at 9:43 AM Phil Boileau notifications@github.com wrote:

Sounds good, thanks. There's no rush!

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/PhilBoileau/cvCovEst/pull/42#issuecomment-782895486, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOFL4VMGU4GTC4LENQTFOEDTAFA4FANCNFSM4X5JO5GA .

PhilBoileau commented 3 years ago

Awesome, thank you for the quick fix. That's a good point. If you have the bandwidth, I think we should make the change sooner rather than later.

bcollica commented 3 years ago

All set, empRiskByClass is now cvRiskByClass. I ran a few tests and all seemed well, but let me know if there are any issues. Changing the column names in the other summary tables will take a bit more care since they are explicitly referenced in other functions, but I can handle that when I add the new matrix stats like condition number. That might also be a good time to solidify a consistent naming convention for future additions.

Brian

On Sun, Feb 21, 2021 at 2:40 PM Phil Boileau notifications@github.com wrote:

Awesome, thank you for the quick fix. That's a good point. If you have the bandwidth, I think we should make the change sooner rather than later.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/PhilBoileau/cvCovEst/pull/42#issuecomment-782940972, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOFL4VMZJVZNML32RIWVLNLTAGDWVANCNFSM4X5JO5GA .

PhilBoileau commented 3 years ago

Perfect, thanks. I'll take a look around, and if all looks well, I'll merge this PR. That sounds like a good plan -- let's plan to discuss this during our next meeting.