Skip to content

Conversation

@eduaguilera
Copy link
Owner

Create gapfilling functions

@eduaguilera eduaguilera requested a review from lbm364dl May 7, 2025 15:15
Copy link
Collaborator

@lbm364dl lbm364dl left a comment

Choose a reason for hiding this comment

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

About the error we saw yesterday in the pkgdown site examples saying that the functions were not found, I couldn't reproduce it, it worked fine for me (after also adding zoo to DESCRIPTION imports). Few things that come to mind:

  • Try to manually delete the man and docs folders, and the NAMESPACE file.
  • Generate again using
    devtools::document()
    devtools::install()
    pkgdown::build_site()
  • Double check if your new functions appear in the NAMESPACE file.
  • Try rerunning code in clean R sessions when something goes wrong, just in case.

I will leave more comments about the code itself after you complete the PR with style fixes and tests.

@lbm364dl
Copy link
Collaborator

If you have time, please have a look at some R packages that might be useful regarding imputation of missing data:

  • imputeTS: imputing missing data on time series.
  • mice: imputing missing data, not focused on time series but on more general multivariate imputation.
  • prophet: focused on predictions, but maybe also works for missing past data? Worth having a look.

There's nothing wrong with using zoo, but maybe something from these packages can be useful, since they have directly available many more imputations besides linear interpolation.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

air

[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

testthat::expect_equal(result$value[result$Source_value == "Original"], c(3, 0, 1, 5))


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

result <- linear_fill(test_data, value, year, category, method = "interpolate")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

interpolated_values <- category_a$value[category_a$Source_value == "Linear interpolation"]


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

result <- linear_fill(test_data, value, year, category, method = "interp_back")


[air] reported by reviewdog 🐶

testthat::expect_false("First value carried backwards" %in% result$Source_value)


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

result <- linear_fill(test_data, value, year, category, method = "interp_forward")


[air] reported by reviewdog 🐶

testthat::expect_true("First value carried backwards" %in% result$Source_value)


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

testthat::expect_true(any(c("Last value carried forward", "First value carried backwards") %in% result$Source_value))


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

# 10 + (20-10) * 2/4


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")
testthat::expect_equal(result$Source_value[2], "First value carried backwards")
testthat::expect_equal(result$Source_value[3], "First value carried backwards")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")
testthat::expect_equal(result$Source_value[2], "First value carried backwards")
testthat::expect_equal(result$Source_value[3], "First value carried backwards")


[air] reported by reviewdog 🐶

result <- linear_fill(grouped_data, value, year, group, method = "interpolate")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

result <- linear_fill(single_value_data, value, year, method = "fill_everything")


[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

proxy_sources <- c("Proxy interpolated", "Proxy carried forward", "Proxy carried backwards")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

change_variable =c(1,2,3,4,1,1,0,0,0,0,0,1)


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

original_rows <- result[result$Source_value == "Original" & !is.na(result$Source_value), ]


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

# Test that functions can be chained together
result1 <- linear_fill(test_data, value, year, category, method = "interpolate")

@lbm364dl
Copy link
Collaborator

Check the formatting issues. It can also be fixed directly from here.

  1. Go to files changed tab.
  2. Search bot comments. Click add suggestion to batch for all of them.
image 3. In the end, commit those suggestions image

This can be handy if you don't have a local air format installation or it's just a few suggestions. Otherwise you should install it and apply the format locally. Follow instructions at https://posit-dev.github.io/air/formatter.html

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

air

[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")
testthat::expect_equal(result$Source_value[2], "First value carried backwards")
testthat::expect_equal(result$Source_value[3], "First value carried backwards")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")
testthat::expect_equal(result$Source_value[2], "First value carried backwards")
testthat::expect_equal(result$Source_value[3], "First value carried backwards")


[air] reported by reviewdog 🐶

result <- linear_fill(grouped_data, value, year, group, method = "interpolate")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

result <- linear_fill(single_value_data, value, year, method = "fill_everything")


[air] reported by reviewdog 🐶

testthat::expect_equal(result$Source_value[1], "First value carried backwards")


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

proxy_sources <- c("Proxy interpolated", "Proxy carried forward", "Proxy carried backwards")


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

change_variable =c(1,2,3,4,1,1,0,0,0,0,0,1)


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

original_rows <- result[result$Source_value == "Original" & !is.na(result$Source_value), ]


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

# Test that functions can be chained together
result1 <- linear_fill(test_data, value, year, category, method = "interpolate")

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

air

[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶


[air] reported by reviewdog 🐶

year = c(2015, 2016, 2017, 2018, 2019, 2020, 2015, 2016, 2017, 2018, 2019, 2020),


[air] reported by reviewdog 🐶

# Test that functions can be chained together
result1 <- linear_fill(test_data, value, year, category, method = "interpolate")

eduaguilera and others added 6 commits September 19, 2025 11:46
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Collaborator

@lbm364dl lbm364dl left a comment

Choose a reason for hiding this comment

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

Great start! I think many of these comments are related to the workflow, so hopefully you'll catch up soon. I only reviewed linear_fill, will check the others later.

@lbm364dl lbm364dl added bug Something isn't working documentation Improvements or additions to documentation duplicate This issue or pull request already exists and removed bug Something isn't working labels Sep 19, 2025
eduaguilera and others added 2 commits September 29, 2025 18:41
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@eduaguilera eduaguilera requested a review from jinfama September 30, 2025 08:00
Copy link
Collaborator

@lbm364dl lbm364dl left a comment

Choose a reason for hiding this comment

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

I think the tests look better now. I managed to read through all of them so I really think they're easier to maintain now. I also agree that the fixtures usage is clean. Most of the comments I left are related to making the code even cleaner, I hope they help, but as I said in them, you don't have to apply all of them if you think your style is better.

An additional question (I think I can't comment on a binary file). Why was the testthat-problems.rds file added? What does it do?

Copy link
Owner Author

@eduaguilera eduaguilera left a comment

Choose a reason for hiding this comment

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

@lbm364dl, I applied your suggestions and I think there is nothing left to be revised.
I added @jinfama as a reviewer because he is also working on gapfilling functions and he has many suggestions and also other functions to propose... @lbm364dl, do you think @jinfama should give his feedback on this PR (it may take a few weeks) or we should close this one and make a new one?

@eduaguilera
Copy link
Owner Author

eduaguilera commented Sep 30, 2025

An additional question (I think I can't comment on a binary file). Why was the testthat-problems.rds file added? What does it do?

About the testthat-problems.rds file, I did not create it on purpose... this is what copilot says:

When it appeared: Git history shows it was first added in commit 999ab85 on 29 Sep 2025 (“revise gapfilling tests”). That commit overhauled test_gapfilling.R and, at the same time, introduced the .rds file.
Why it exists: The RDS holds a list of eight expectation_error objects. testthat automatically writes testthat-problems.rds whenever a test run finishes with failures or other “problems.” It’s meant to be a scratch file so the next run (or snapshot_review()) can remind you about unresolved issues. Normally it stays untracked, but because it wasn’t ignored, it ended up committed along with the test refactor.

Shall I delete it and add it to .gitignore?

@lbm364dl
Copy link
Collaborator

lbm364dl commented Oct 1, 2025

I added @jinfama as a reviewer because he is also working on gapfilling functions and he has many suggestions and also other functions to propose... @lbm364dl, do you think @jinfama should give his feedback on this PR (it may take a few weeks) or we should close this one and make a new one?

When we're done with my suggestions we can merge. @jinfama can add his review when he has time, and then we can create an issue with his suggestions and do them in a new PR. Even if a PR is merged/closed, you can still add your review or comments.

About the testthat-problems.rds file, I did not create it on purpose...

Shall I delete it and add it to .gitignore?

You have no snapshot tests in the current code. Maybe you tried to add one at some point. Anyway, I think this file shouldn't appear again if you currently have no snapshot tests. Just delete the file, no need to add to .gitignore for now.

Copy link
Collaborator

@lbm364dl lbm364dl left a comment

Choose a reason for hiding this comment

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

A few small comments. We're almost there!

PS: Remember to also remove the testthat-comments.rds file.

@lbm364dl lbm364dl force-pushed the edu/add-old-functions branch from 7370da9 to 9015863 Compare October 7, 2025 14:41
@eduaguilera eduaguilera changed the title Edu/add old functions Edu/add gapfilling functions Oct 7, 2025
@lbm364dl lbm364dl changed the title Edu/add gapfilling functions Add gapfilling functions Oct 7, 2025
Copy link
Collaborator

@lbm364dl lbm364dl left a comment

Choose a reason for hiding this comment

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

LGTM! Great job for the first PR!

@eduaguilera eduaguilera merged commit 48d77a6 into main Oct 7, 2025
9 checks passed
@lbm364dl lbm364dl added this to the v0.2.0 milestone Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants