Skip to content

Conversation

@certara-irebai
Copy link
Contributor

This plot displays the sequence of estimates for population parameters computed after each iteration of the SAEM algorithm from Monolix output

@A2P2
Copy link
Collaborator

A2P2 commented Jan 9, 2025

Hi @certara-irebai,
Thanks, it works for me when I added the CvParam.txt to the testdata.
image

Some thoughts:

  1. Let's add ChartsData/Saem/CvParam.txt to the testdata folder, so that the this function work with the theophyline example.
  2. Expand the test with the Saem plot here:
    test_that("We can call all pmx_plot_xx without title with success", {
  3. This function should only work for monolix. Should there be a check for it?
  4. Could you check if you can create the ggplot object using default ggPMX plot style. https://github.com/ggPMXdevelopment/ggPMX/blob/bd679e25ffc50f22ae2773587a44bca50f24ede6/R/pmx-all-plots.R
  5. You now parse the SAEM data in the plotting call. Shouldn't it be parsed when controller is created?
  6. Regarding additional arguments to the function, monolix has ncol (integer) number of columns when perhaps it should be added here as well.

Overall you provided very clearly written code. My worry is that implementation is a bit 'ad-hoc' and doesn't follow the previous coding style in ggPMX. @mattfidler What do you think? Shall we have the ad-hoc implementation or shall it be better integrated?

Best,
Alex

@A2P2
Copy link
Collaborator

A2P2 commented Jan 9, 2025

Fixes #152

@mattfidler
Copy link
Collaborator

mattfidler commented Jan 9, 2025

Hi @certara-irebai

This is a good first step in creating the saem convergence plot. For ggPMX our goal is to support NONMEM, nlmixr2 and Monolix. This will only work with Monolix so it needs a bit of work to allow this to happen:

  • First the data for the convergence plot type needs to be accommodated in the R6 object (this would be the saem convergence data as well as the transition points)
  • Second, the data import needs to be done in the controller creation section (with a possibility that these files do not exist and do not produce the data needed, which also needs to be tested)
  • Third, a generic plot type like the convergence plot needs to be created
  • Fourth, the generic plot type needs to be called with yaml, which would allow the user to replace plot titles with a custom yaml definition.

Again, congratulations on getting this piece done. Just a bit more to get it past the finish line.

And of course, the tests need to pass 😉

@mattfidler
Copy link
Collaborator

Hi @certara-irebai,

@A2P2 showed me the reason that the CI may be failing. I will be creating a branch where this is fixed.

@mattfidler
Copy link
Collaborator

Note the class definition that will need to take more data is here:

pmxClass <- R6::R6Class(

@mattfidler
Copy link
Collaborator

To load the data, you change the yaml files:

inst/templates/mlx/standing.ipmx

@mattfidler
Copy link
Collaborator

If you need help with the third or fourth items, please let me know.

@A2P2 A2P2 changed the base branch from master to develop January 14, 2025 12:09
@A2P2 A2P2 closed this Jun 16, 2025
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.

3 participants