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

Don't require model modification to set initial conditions #15

Closed
billdenney opened this issue Mar 9, 2024 · 9 comments · Fixed by #16
Closed

Don't require model modification to set initial conditions #15

billdenney opened this issue Mar 9, 2024 · 9 comments · Fixed by #16

Comments

@billdenney
Copy link
Collaborator

Using a model that sets initial conditions (e.g. pd(0) <- initial) causes an error with the targets package (see ropensci/targets#876).

Have nlmixr2targets automatically run nlmixr2() on the object in-place so that it's not detected by targets as having an invalid syntax. An alternative option is to rewrite the model in-place to replace any initial condition setting with the current pd(initial) <- initial style used in the development version of nlmixr2targets.

@mattfidler
Copy link
Member

I vote for using nlmixr2 parsed models in case something is introduced in targets or nlmixr2 that doesn't play well together

@billdenney
Copy link
Collaborator Author

That is the path that I plan to pursue. I just need to be sure that it doesn't introduce any extra target running. The underlying issue there would be if the object digest changed from run-to-run.

If digest changes are a problem, the better solution there would be to ensure that the digest did not change, but I've had some issues with that in the past due to the environment creation within nlmixr2est and rxode2.

@billdenney
Copy link
Collaborator Author

This gets very hard very quickly with the possibility of piped models. The object in an tar_nlmixr() call could be a piped model such as pheno |> ini(lcl = log(0.1)) in which case searching for it correctly would become hard. I may need to search each object in the call to see if it may be an nlmixr2/rxode2 model function and then call those.

Are there any scenarios where an rxUi object would not work in a pipeline while a function would? (I think no-- or if yes, it should be fixed in piping. But, I'm not sure.)

@mattfidler
Copy link
Member

mattfidler commented Mar 9, 2024

As long as it is assigned to an object it will be fine. If it goes through a pipeline into the target_nlmixr() it will be a compressed rxUi object. So the logic should be:

  • Get the object name from substitute
  • Use rxode2::assertRxUi() to convert an object to the correct form.
  • I can't remember, but I think the assert returns an uncompressed rxUi. To be sure it is not an environment use rxode2::rxUiCompress()
  • If the object exists in the prior environments, assign it to the parsed, compressed object

This should let it work with anything that can be converted to the UI object

@mattfidler
Copy link
Member

mattfidler commented Mar 9, 2024

Note piped objects will not exist in the environment.

The only issue I see is when a rxode2 function is in the environment but not evaluated. I guess you could loop over everything that is a function and try to evaluate to a ui object. If it works save it in the environment. This isn't ideal but could work.

@mattfidler
Copy link
Member

Are there any scenarios where an rxUi object would not work in a pipeline while a function would? (I think no-- or if yes, it should be fixed in piping. But, I'm not sure.)

No

@billdenney
Copy link
Collaborator Author

I ended up looping over everything in the call rather than everything in the environment. That will hopefully catch anything that comes up.

Would you mind taking a look at the PR to see if anything jumps out at you as an issue?

@mattfidler
Copy link
Member

Sure. I might add a test case to see if I can break it. I will probably do it by next week.

@billdenney
Copy link
Collaborator Author

Thanks. I'm going to merge the PR since the next feature will change code in similar places.

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 a pull request may close this issue.

2 participants