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.

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

Rewrite README and style code #155

Merged
merged 27 commits into from
Oct 1, 2018
Merged

Rewrite README and style code #155

merged 27 commits into from
Oct 1, 2018

Conversation

peterdesmet
Copy link
Collaborator

This PR includes updates to prepare for rOpenSci onboarding (#148). The main changes are:

  • Run styler package over the code to clean up whitespace etc.: this is why most R files are updated
  • Add codemeta.json (for rOpenSci)
  • Add URLs for authors (website for @adokter, twitter for @peterdesmet and @stijnvanhoey)
  • Update intro to package in bioRad.R and DESCRIPTION (as discussed in Review README #138)
  • Generate README.md from README.Rmd (allows to have code examples)
  • Rewrite the README

✨New README ✨

The new README now has:

  • Intro
  • Installation
  • Usage
  • Meta (was already there)

All elements of the original README are included in one way or another, except:

What has been added:

  • Docker instructions are collapsed by default
  • Usage example for radar data
    • Where you can find open NEXRAD data (including AWS)
    • Plot of VRADH + explain a bit
  • Usage example for vp data
  • All examples use easier to read (dplyr) pipe format

@adokter @CeciliaNilsson709 can you review the README to see if it is clear and that especially for the examples I'm not saying something incorrect? 😅

Copy link
Owner

@adokter adokter left a comment

Choose a reason for hiding this comment

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

Not sure I added comments/review to this PR properly, but the bottom line:

In README.md:

  • I made a few edits to the example of height-integrated time series. I want to avoid people averaging mtr's. Instead they should use the time-integrated column mt (accounts properly for the potentially varying time intervals in the time series)
  • Thought it would be nice to also include a plot of the height-integrated time series. Added it to the readme, but not yet the figure
  • regularize_vpts no longer essential, though without it you do not see the gray bars without data
  • should we really use tidyverse piping in the examples? I haven't tested yet whether all package functions are compatible

@adokter
Copy link
Owner

adokter commented Sep 27, 2018

#128 documents another windows-specific issue that some users ran into, which would be good to document together with other known Docker issues

@peterdesmet
Copy link
Collaborator Author

Will have a look. 👌 Should have let you know to edit the README.Rmd rather that the automatically created README.md.

@peterdesmet
Copy link
Collaborator Author

@adokter

  • Rewrote vpi example a bit, explaining it is a dataframe
  • Mentioned as comment that integrate_profile also regularizes it (at least, that's how I understood your comment)
  • Included plot (I had it in a first draft, but it makes sense to include it)
  • Yes, I think we should include piping even though it hasn't been fully tested. It's a concept that makes code much more readable, so it's good to have it in the examples (and people might learn from it).

I'll include the installation gotchas in #154 and #128 (which still need a summary) in a later PR if that's ok.

Already included via DESCRIPTION when doing ?bioRad
@adokter
Copy link
Owner

adokter commented Sep 27, 2018

Mentioned as comment that integrate_profile also regularizes it (at least, that's how I understood your comment)

Sorry I wasn't clear. Without regularizing you don't see data gaps in the plot, it just fills the area between consecutive profiles, even though they may be widely spaced in time during radar downtime. Often that's fine, especially if there is no missing data.

Regularizing has the benefit that you specify at which interval you expect profiles, so I can replace periods with missing data with empty profiles, that show up as gray.

@peterdesmet
Copy link
Collaborator Author

@adokter So, without regularize a data point in the plot is carried through (straight line) until the chart continues? While with regularize, datetimes are added, resulting in more rows (and actual gaps NA in the plot)?

> example_vpts %>% integrate_profile() %>% nrow()
[1] 1934
> example_vpts %>% regularize_vpts %>% integrate_profile() %>% nrow()
projecting on 300 seconds interval grid...
[1] 2735

If yes, than I think it is good to still include regularize_vpts in the first example (to see the gray bars). Also, not using it gives a warning when plotting.

Not sure what we should say about or include it in the vpi plot... but my incorrect comment needs to be updated:

Note: integrated profiles are regularized automatically

@peterdesmet peterdesmet added this to the 0.4.0 milestone Sep 28, 2018
@peterdesmet peterdesmet self-assigned this Sep 28, 2018
@adokter
Copy link
Owner

adokter commented Sep 29, 2018

@adokter So, without regularize a data point in the plot is carried through (straight line) until the chart continues? While with regularize, datetimes are added, resulting in more rows (and actual gaps NA in the plot)?

Yes

@adokter
Copy link
Owner

adokter commented Sep 30, 2018

@peterdesmet should I merge this PR?

@peterdesmet
Copy link
Collaborator Author

I removed my incorrect remark: PR can be merged now. 👌

Might be good to give some more explanation in the regularize_vpts function documentation, explaining its effect on plots.

@adokter adokter merged commit 00156e9 into master Oct 1, 2018
@adokter adokter deleted the onboarding branch October 1, 2018 14:15
@adokter
Copy link
Owner

adokter commented Oct 1, 2018

Had to make a few additional commits to master, since some commits entered the develop branch instead of this PR request. Also and ran devtools check and rebuilt pkgdown site. Consider 7451eb1 the end of this PR.

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.

2 participants