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.

Sign up for GitHub

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

Jump to bottom

remove support for survey.design objects #36

Merged
merged 11 commits into from Apr 19, 2020
Merged

remove support for survey.design objects #36

merged 11 commits into from Apr 19, 2020

Conversation

GregorDeCillia
Copy link
Contributor

@GregorDeCillia GregorDeCillia commented Apr 17, 2020 •
edited by alexkowa

only implement the imputation functions for objects of type data.frame (and data.table) and drop support for survey.design objects.

  • remove method dispatch and only expose one function for each imputation method
  • properly test if data.table causes any issues
  • Make sure the documentation is in-line with these changes
  • do some typechecking and implement a special "deprication error" if an object of type survey.design is passed for data
  • (possibly) remove survey support also for the visualization functions and remove the package methods from the upstream dependencies
  • check if VIM has any downstream dependencies on CRAN which use the survey interface. @alexkowa or @matthias-da: do you know how to do that automatically? I know there is some package for reverse dependency checks on r-lib ...

@GregorDeCillia
remove survey support for imputation functions
c7ace79
@GregorDeCillia
fix tests
f32375b 
since data.table uses reference semantics rather
than lazy data structures, it is necessary to
use the force() to make sure the parameters
dist_var and variable are evaluated before the
object data is manipulated in the function body
of kNN.

If the return value does not have the correct
type (R/kNNFaster#185), then indexing does
not behave as expected in one of the tests
(tests/test_kNN.R#65)
@coveralls
Copy link

coveralls commented Apr 17, 2020 •
edited

Coverage Status

Coverage increased (+1.3%) to 61.734% when pulling 7d23978 on rm_survey into 0cac771 on master.

@GregorDeCillia GregorDeCillia added this to the VIM 6.0.0 milestone Apr 17, 2020
@GregorDeCillia GregorDeCillia self-assigned this Apr 17, 2020
@GregorDeCillia GregorDeCillia added the ns namespace label Apr 17, 2020
@GregorDeCillia GregorDeCillia changed the title remove support for survey objects remove support for survey.design objects Apr 18, 2020
@GregorDeCillia
Copy link
Contributor Author

There are currently 10 S3 methods left

VIM/NAMESPACE

Lines 3 to 12 in 3670041

S3method(aggr,data.frame)
S3method(aggr,default)
S3method(aggr,survey.design)
S3method(plot,aggr)
S3method(prepare,data.frame)
S3method(prepare,default)
S3method(prepare,survey.design)
S3method(print,aggr)
S3method(print,summary.aggr)
S3method(summary,aggr)

I suggest we also remove aggr.*() and prepare.*() and reduce this to
the 4 *.aggr() methods.

@GregorDeCillia
Copy link
Contributor Author

@alexkowa or @matthias-da can you run revdepcheck::revdep_check() for this branch to see if there are any issues with reverse dependencies? I can't get this to work at the moment.

@GregorDeCillia GregorDeCillia marked this pull request as ready for review April 18, 2020 17:22
@alexkowa
Copy link
Member

revdepcheck is fine.
`
─ CHECK ────────────────────────────── 8 packages ──

✓ destiny 3.0.1 ── E: 1 | W: 1 | N: 8

✓ micemd 1.6.0 ── E: 0 | W: 0 | N: 0

✓ missCompare 1.0.1 ── E: 0 | W: 0 | N: 0

✓ robCompositions 2.2.1 ── E: 0 | W: 1 | N: 0

✓ sdcMicro 5.5.1 ── E: 0 | W: 0 | N: 0

✓ simPop 1.2.0 ── E: 0 | W: 0 | N: 0

I simputation 0.2.4 ── E: 1 | W: 0 | N: 0

✓ smartdata 1.0.3 ── E: 2 | W: 0 | N: 1
`

Errors seem not to be VIM related (so no change in check old/new

alexkowa
alexkowa approved these changes Apr 19, 2020
View reviewed changes
Copy link
Member

@alexkowa alexkowa left a comment

Choose a reason for hiding this comment

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

Go ahead an merge f

@GregorDeCillia GregorDeCillia merged commit b7227f0 into master Apr 19, 2020
@GregorDeCillia GregorDeCillia deleted the rm_survey branch April 19, 2020 18:24
@GregorDeCillia
Copy link
Contributor Author

thanks! I decided to go for a "squash and merge" because some of the commit messages expose unnecessary details for the main commit history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexkowa alexkowa alexkowa approved these changes

@matthias-da matthias-da Awaiting requested review from matthias-da

Assignees

@GregorDeCillia GregorDeCillia

Labels
breaking ns namespace
Projects
None yet
Milestone
VIM 6.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@GregorDeCillia @coveralls @alexkowa