I1378

[tinigarske]

Link to youtrack issue

https://vimc.myjetbrains.com/youtrack/issue/VIMC-1378

This PR makes some suggestions for improving the modup documentation and cleaning up now obsolete bits of code - it shouldn't be merged in as is.

(Is a PR a good way of communicating such suggestions?)

Things you would particularly like Rich to look out for

1. modup_doc.md - this is the kind of information I'd like to have available when I access the modups in the reporting portal. I think it's unlikely this is the right place to keep this documentation, but am not sure how to best handle it. Would it be possible to spit this out into the report whenever we do a modup that generates the files described here? [Also, beware of my markdown-markup, it's probably rather dodgy - how can you display it? Chrome just gives me raw text which isn't super helpful...]

2. What's your thoughts on streamlining the generation of impact rates down to only generate those that we'll use in modup method 2 going forward? Most of them we don't need anymore.

Things you would particularly like Xiang to look out for

Could you implement the changes based on my suggestions and Rich's feedback on my points above?
There's one more suggestion regarding small countries - would be great to double check with Wes if we have the info on Montagu and therefore can remove the restriction.

[xiangli313]

Sure @tinigarske. I will make any changes needed.

I'd like to refer to one of your comments - "apparently MHL, TUV and XK are excluded from the modup because we didn't used to have population data from UNWPP, but I believe that Wes has synthesized that, so we should be able to include that now."

We do have population data for these small countries now. However, population statistic is touchstone specific. Any touchstone before 201710gaiv does not have population data for these small countries. So we have to keep the bit of code you removed.

[richfitz]

modup_doc.md - this is the kind of information I'd like to have available when I access the modups in the reporting portal. I think it's unlikely this is the right place to keep this documentation, but am not sure how to best handle it. Would it be possible to spit this out into the report whenever we do a modup that generates the files described here? [Also, beware of my markdown-markup, it's probably rather dodgy - how can you display it? Chrome just gives me raw text which isn't super helpful...]

For now, this is great. The only thing that needs doing is to add ^modup_doc\.md$ to .Rbuildignore. The markdown will render as soon as it's on github: https://github.com/vimc/jenner/blob/i1378/modup_doc.md - as you can see it looks great. There are "proper" ways of getting documentation into packages (this would probably become a vignette) but for now I think this is good because it has a low barrier to working on it and it will be more prominent in the jenner github home.

What's your thoughts on streamlining the generation of impact rates down to only generate those that we'll use in modup method 2 going forward? Most of them we don't need anymore.

Consider keeping a copy of the currently working code as a "legacy impact" and work with the new method going forward perhaps? Or just note somewhere which is the last version that supports the previous method. "Most of them" is different to never, which could end up biting us

[tinigarske]

Thanks both for your feedback!

Regarding small countries - I get your point, @xiangli313 . Would the thing to do then be to put an if-clause around it that discriminates between touchstones?

For impact rates - going forward, we will always use method 2 (or 3 once we have that), never method 1. Method 2 uses only the total impact rate, not the inst or avg rates. So I think we are in the never situation (sorry @richfitz for not being clear). Having a "legacy modup" code version seems a good idea.

[tinigarske]

So, it seems to me that this is fine now, and ready to merge in (although there's something with the CI going on that I have no clue about - but that must have come from master, so should be ok, right?)

@richfitz, would you mind merging this in for real?