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

spatial data #29

Merged
merged 73 commits into from
Mar 4, 2018
Merged

spatial data #29

merged 73 commits into from
Mar 4, 2018

Conversation

nevrome
Copy link
Member

@nevrome nevrome commented Feb 6, 2018

PR to keep track of the changes

Hamer and others added 8 commits November 16, 2017 17:31
- the attempt so far does a spatial join based on the world countries
and receives the official country name
- afterwards this name is
compared to the country code in the c14datelist object
- a check column is tested for NAs
- a NA can be caused by:

1. a wrong code (as is the case for the mentioned codes vs the iso3166
codes); due to this reason the join is based on the UN codes
2. a code, that is not correctly assigned (since the written codes are part of the UN code list)
3. a missing code, what is also often the case.

To Do:
- The approach so far is a little messy and should be broken in
smaller steps (for better reproducibilty and to not introduce further
bugs)
- check the iso datatables for consistency; perhaps provide an own
table that is state of the art.
Next steps: Continue with longitude
@codecov-io
Copy link

codecov-io commented Feb 6, 2018

Codecov Report

Merging #29 into master will decrease coverage by 6.57%.
The diff coverage is 3.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   78.31%   71.74%   -6.58%     
==========================================
  Files          24       28       +4     
  Lines        1033     1129      +96     
==========================================
+ Hits          809      810       +1     
- Misses        224      319      +95
Impacted Files Coverage Δ
R/c14_date_list_spatial_standardize_country_name.R 97.67% <ø> (ø)
R/c14_date_list_spatial_all_spatial_functions.R 0% <0%> (ø)
...ate_list_spatial_determine_country_by_coordinate.R 0% <0%> (ø)
R/c14_date_list_spatial_coordinate_precision.R 0% <0%> (ø)
R/c14_date_list_spatial_finalize_country_name.R 0% <0%> (ø)
R/c14_date_list_duplicates_mark.R 94.87% <100%> (ø) ⬆️
R/c14_date_list_order_variables.R 97.22% <100%> (+0.07%) ⬆️
R/c14_date_list_enforce_types.R 95.83% <100%> (ø) ⬆️
... and 2 more

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 4e9013d...b93be41. Read the comment docs.

@dakni
Copy link
Contributor

dakni commented Feb 6, 2018

I would not yet merge "coordinate_precision.R" since we are still in the phase of writing/finishing/testing the functions;

the file from the playground should be deleted before merging

the file "determine_country_by_coordinate.R" is ok to be merged, though it is also still undertested.

@nevrome
Copy link
Member Author

nevrome commented Feb 6, 2018

As the title of the PR already says: Do not merge yet

I just really like the PR interface of github to track changes. :-)

@dakni
Copy link
Contributor

dakni commented Feb 6, 2018

ahja... ;)

in this case: how is it possible to indicate what to be merged and what not? is it possible to review an entire file or am can only review single lines?

and...what do you mean by tracking changes? where is the difference to a simple commit log!?

@nevrome
Copy link
Member Author

nevrome commented Feb 6, 2018

I'm not aware of a way to exclude certain files or commits from a PR on github. You would need to split that up into different branches and PRs.
The review can happen line wise or concerning the complete PR. No review on file level (but why would you need that if you can work on every line?).
The commit log is fine. In this case I just wanted to see all the new stuff in direct relation to the master branch. The automatic checks/tests also already consider the master branch here.


# check if the columns country_coord and country_thes are present
necessary_vars <- c("country_coord", "country_thes")
x %>% check_if_columns_are_present(necessary_vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

there is something missing here; where does the pipe lead to?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was hoping, that if the check was positive it wouldn´t have to lead anywhere and just move on and if the test is negative the function would be stopped by the check

Copy link
Contributor

@dakni dakni left a comment

Choose a reason for hiding this comment

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

nice and thanks. let's test this tomorrow during the ISAAK meeting.

# pick the most likely result hierarchically from country_coord, country_thes
# and the original input data
x %<>%
dplyr::mutate(
if (country_coord != NA) {
country_final = country_coord
} else if (country_thes ! NA) {
} else if (country_thes != NA) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you do not need else if

either use elseor if

Schmuetz and others added 5 commits February 8, 2018 17:32
todo:
- clean precision calculations
- change precision functions to helper functions
- document text
no "resort" of the lines in the csv but just an adjustment of the
order of the columns; should be double checked for consistency
@nevrome
Copy link
Member Author

nevrome commented Feb 20, 2018

Is your work ready for a review?

Wolfgang Hamer and others added 27 commits March 2, 2018 14:10
Merge branch 'master' into SQE

# Conflicts:
#	NAMESPACE
#	R/c14_date_list_clean.R
@nevrome nevrome merged commit 47a7611 into master Mar 4, 2018
@nevrome nevrome changed the title Do not merge yet: spatial data spatial data Mar 4, 2018
@nevrome nevrome deleted the SQE branch March 13, 2018 22:58
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.

4 participants