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

A few suggestions #1

Open
bguiastr opened this issue Dec 6, 2017 · 3 comments
Open

A few suggestions #1

bguiastr opened this issue Dec 6, 2017 · 3 comments
Assignees

Comments

@bguiastr
Copy link

bguiastr commented Dec 6, 2017

Good job with this integration, I have noticed the following when testing xpose.nlmixr:

  • In the column index of the data the columns CWRES and CPRED are assigned to a type NA, which causes the list_vars() function to output an NA at the end
  • In the xpdb general structure, you added an xpdb$software level that is not part of the structure of xpdb structure. As we will need to use more and more if(software == ...){} in the code this might actually become be handy, alternatively I was thinking of adding an internal utility function software(xpdb) let me know what you think.
  • The get_code()function does not work as the xpdb does not contain any code, you could add the code from the model function if possible?!
  • The get_prm() and hence prm_table() functions do not work, this might be my fault this function is dependent on NONMEM output so I will try to improve on that part when I find the time.
@kestrel99
Copy link
Collaborator

Thanks! Answers to your questions -

  • Will fix CWRES and CPRED. What should type be set to?
  • I agree something is needed - I added the software tag because it seemed natural to me, but will defer to you and the xpose team on how best to implement.
  • get_code(): will do
  • get_prm() and prm_table(): Let me know if I need to do anything here

@bguiastr
Copy link
Author

bguiastr commented Dec 6, 2017

  • the type of CWRES should be 'res', and CPRED could be 'na' preferably or 'pred' depending if you want it by default. For NONMEM we opted for PRED as the default 'pred' var as CPRED is not always present, maybe it is better to be consistent across tools?
  • I added a software function in the last PR so you can remove the $software tag. Right now Seb and I are thinking about the best way of having this in the future (i.e. if(software ==) or S3 methods with software used as class on xpdb objects) but there is no easy solution for now.
  • get_prm(): I need to look into it first but I will keep you updated when I do so.

@kestrel99 kestrel99 self-assigned this Dec 8, 2017
@kestrel99
Copy link
Collaborator

CRES, CWRES and CPRED: fixed. get_code(): fixed. (Both in today's #commits)

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

No branches or pull requests

2 participants