Skip to content

Conversation

lrennels
Copy link
Collaborator

@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #796 (f27be2b) into master (8d8ec3b) will increase coverage by 0.89%.
The diff coverage is 88.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
+ Coverage   82.97%   83.87%   +0.89%     
==========================================
  Files          39       39              
  Lines        3466     3473       +7     
==========================================
+ Hits         2876     2913      +37     
+ Misses        590      560      -30     
Flag Coverage Δ
unittests 83.87% <88.23%> (+0.89%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/mcs/defmcs.jl 90.00% <88.23%> (+27.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d8ec3b...f27be2b. Read the comment docs.

@lrennels lrennels changed the title Add an error for replace_RV when name doesn't exist RV modification functions Mar 18, 2021
@lrennels lrennels requested a review from arnavgautam March 18, 2021 21:31
@lrennels
Copy link
Collaborator Author

@grantmcdermott this should help with some of the issues you ran into here: anthofflab/MimiPAGE2009.jl#222

@grantmcdermott
Copy link

grantmcdermott commented Mar 18, 2021

Super, thanks for the head up @lrennels!

Two minor aside, which I hope you don't mind me mentioning here although they're only tangentially related to this PR.

  • Assuming this is still the case, could you mention somewhere in the documentation that EmpiricalDistribution isn't exported by default when loading Mimi?
  • Talking documentation, is there a reason why "Documentation" isn't a main sub-menu link on the left-hand side of the Mimi website?

@lrennels
Copy link
Collaborator Author

@grantmcdermott I don't mind at all, feedback is really helpful. Per your first comment yes that's a good idea I'll add some documentation on the status of EmpiricalDistribution, need to go look at if there's a reason we haven't documented it (i.e. it's in Beta format and we don't want to throw users in the deep end unless they're comfortable with it), or we just hadn't done it yet. I think I didn't put it in the sub-menu link since those are mostly links to pages within the forum as opposed to external links, but if you think it would be helpful I could put it there! I'm not sure the left hand side label can be a link, but it could link to a page that provides the link.

@lrennels
Copy link
Collaborator Author

@grantmcdermott just added a documentation link to the left-side bar, good idea!

arnavgautam
arnavgautam previously approved these changes Mar 29, 2021
Copy link
Collaborator

@arnavgautam arnavgautam left a comment

Choose a reason for hiding this comment

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

Always exciting to see us surfacing more details on errors

wip/temp.jl Outdated
@@ -0,0 +1,34 @@
using Mimi
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this file intended to be checked in? I can't see how it connects to the rest of the files in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah good point thank you, deleted it!

@lrennels lrennels merged commit 0be5ab3 into master Mar 30, 2021
@lrennels lrennels deleted the rv branch March 30, 2021 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants