Skip to content

adding get_values to CategoricalVariable and making a new function for IntegrerVariable#207

Merged
JasonRWood merged 6 commits intomrc-ide:devfrom
JasonRWood:feat/cat_get_values_int_get_modulo_differences
Aug 7, 2025
Merged

adding get_values to CategoricalVariable and making a new function for IntegrerVariable#207
JasonRWood merged 6 commits intomrc-ide:devfrom
JasonRWood:feat/cat_get_values_int_get_modulo_differences

Conversation

@JasonRWood
Copy link
Contributor

This PR adds the get_values function to variables of the type CategoricalVariable and a new function called get_modulo_differences to variables of the type IntegerVariable.

get_modulo_differences is designed to be used in future work where individuals within host dynamics may only want to be updated every N timesteps.

get_values has been added to CategoricalVariable to be able to follow the changes that occur for a given individual more easily.

@JasonRWood JasonRWood requested a review from giovannic August 5, 2025 15:04
Realised I'd left a commented out line that was part of an earlier idea
Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

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

Wonderful. I would add some simple tests to protect this code from future changes. Something like:

in test-categoricalvariable.R

test_that("CategoricalVariables returns correct values", {
  categories = c('S', 'S', 'R', 'I')

  # standard case
  variable <- CategoricalVariable$new(SIR, categories)
  expect_equal(categories$get_values(), categories)

  # empty case
  variable <- CategoricalVariable$new(SIR, c())
  expect_equal(variable$get_values(), c())
})

test_that("CategoricalVariables returns correct values after update", {
  categories = c('S', 'S', 'R', 'I')

  # standard case
  variable <- CategoricalVariable$new(SIR, categories)
  variable$queue_update(1, 'I')
  variable$.update()
  expect_equal(variable$get_values(), c('I', 'S', 'R', 'I'))
})

# perhaps in here: https://github.com/mrc-ide/individual/blob/dev/tests/testthat/test-categoricalvariable-resize.R
test_that("CategoricalVariables returns correct values after resize", {
  categories = c('S', 'S', 'R', 'I')

  # standard case
  variable <- CategoricalVariable$new(SIR, categories)
  x$queue_extend(values = rep('I', 2))
  x.resize()
  expect_equal(variable$get_values(), c(categories, rep('I', 2)))
})

And for integer variable tests

test_that("IntegerVariables returns correct modulo differences", {
  #...
  expect_equal(variable$get_modulo_differences(x, d)$to_vector(), expected_modulo_differences)
})

test_that("IntegerVariables errors for invalid modulo differences", {
  #...
  expect_error(variable$get_modulo_differences(x, -1))
  expect_error(variable$get_modulo_differences(wrong_sized_values, d))
})

And finally, please run devtools::document() in the root directory and commit the updated documentation so that it appears on the website when this merges!

Copy link
Member

@giovannic giovannic left a comment

Choose a reason for hiding this comment

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

I've approved, but please remove the comment and extra unnecessary checks before merging (I've pointed them out as suggestions). Thank you!

@@ -1,62 +1,72 @@
SIR <- c('S', 'I', 'R')
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, why is all this diffed?

Copy link
Member

Choose a reason for hiding this comment

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

My guess is that your git is changing all the hiddne line endings

I would recommend this in the future: git config --system core.autocrlf false

Stack overflow for context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the fix

Co-authored-by: Giovanni <giovanni.charles@gmail.com>
@JasonRWood JasonRWood merged commit 71a7916 into mrc-ide:dev Aug 7, 2025
5 checks passed
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