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

587 get seed #607

Merged
merged 3 commits into from
Sep 9, 2021
Merged

587 get seed #607

merged 3 commits into from
Sep 9, 2021

Conversation

msevestre
Copy link
Member

No description provided.

@msevestre msevestre requested a review from PavelBal September 8, 2021 20:29
@@ -90,8 +90,6 @@ Collate:
'table-formula.R'
'user-defined-pk-parameter.R'
'utilities-container.R'
'utilities-data-import-configuration.R'
'utilities-data-importer-configuration.R'
Copy link
Member Author

Choose a reason for hiding this comment

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

@PavelBal I am not sure what these files were about? They were empty or containing an empty function

population <- Population$new(netPopulation)

individualCharacteristics <- NULL

# NOTE THIS IS A WORKAROUND UNTIL THE CODE IN PKSIM IS UPDATED
Copy link
Member Author

Choose a reason for hiding this comment

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

Also fixed this in PKSim

if (len == 0) {
rep(NA, length(dataSet$xValues))
} else if (len == 1) {
columForDataSet <- function(dataSet) {
Copy link
Member Author

Choose a reason for hiding this comment

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

just running styler here

@@ -1,6 +1,8 @@
context("New DataImporterConfiguration")

test_that("it can create a new data importer configuration", {
skip_on_os("linux") # TODO enable again as soon as npoi works under linux
Copy link
Member Author

Choose a reason for hiding this comment

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

this was missing everywhere too

@@ -1,6 +1,8 @@
context("New DataImporterConfiguration")

test_that("it can create a new data importer configuration", {
skip_on_os("linux") # TODO enable again as soon as npoi works under linux

importerConfiguration <- DataImporterConfiguration$new()
Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly do not like the fact that so much happens on new. A constructor should be very fast and NEVER crash.
I would like to see a method on that object for importing or creating. Right now this is all implicit and it is not what I would expect

@msevestre msevestre requested a review from Yuri05 September 8, 2021 20:33
@PavelBal
Copy link
Member

PavelBal commented Sep 9, 2021

Ah ok, the dlls must be updated for the build to success.

@msevestre msevestre merged commit d996bd5 into develop Sep 9, 2021
@msevestre msevestre deleted the 587-get-seed branch September 9, 2021 15:43
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