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

114 inconsistency between fitted and predict #115

Merged
merged 5 commits into from
Jan 8, 2024

Conversation

mahendra-mariadassou
Copy link
Collaborator

Fix #114 by changing behavior of predict() method (and corresponding S3-methods) for PLNfit objects:

  • if newdata argument is missing, returns fitted
  • if newdata is provided but not responses, returns either $EZ = XB + O$ (link type) or $EZ = exp(XB + O + 0.5\text{diag}(\Sigma))$ (response type) as the best estimates of $M_{new}$ and $S_{new}$ are then 0 and $\text{diag}(\Sigma)$
  • if responses is also provided, performs one VE step to estimate $M_{new}$ and $S_{new}$ and returns either $EZ = XB + O + M_{new}$ (link type) or $EZ = exp(XB + O + M_{new} + 0.5 S_{new})$ (response type)

Open question for @jchiquet : should we return $M_{new}$ and $S_{new}$ as attributes of the predictions (like we do for predict_cond() ? I think there's not a lot of added value compared to using optimize_vestep() if one is mostly interested in M and S but you may have a different opinion.

@jchiquet
Copy link
Member

jchiquet commented Jan 7, 2024

Open question for @jchiquet : should we return and as attributes of the predictions (like we do for predict_cond() ? I think there's not a lot of added value compared to using optimize_vestep() if one is mostly interested in M and S but you may have a different opinion.

I agree with you, no need to send back M or S as attributes.

Copy link
Member

@jchiquet jchiquet left a comment

Choose a reason for hiding this comment

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

  • change to 1.1.0
  • check comment on adding .5 * S² or not

NEWS.md Outdated Show resolved Hide resolved
DESCRIPTION Outdated Show resolved Hide resolved
## mean latent positions in the parameter space
EZ <- X %*% private$B
if (!is.null(O)) EZ <- EZ + O
EZ <- sweep(EZ, 2, .5 * diag(self$model_par$Sigma), "+")
Copy link
Member

Choose a reason for hiding this comment

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

I see you do not add this back in the update version, so it was indeed a mistake?

Copy link
Collaborator Author

@mahendra-mariadassou mahendra-mariadassou Jan 8, 2024

Choose a reason for hiding this comment

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

Yes, I looked at predict_cond() and it made me realize that $E[Z]$ should not include any variance term (unlike $E[Y]$) so I changed the code accordingly.

PLNmodels/R/PLNfit-class.R

Lines 560 to 564 in 4a68804

results <- switch(
type,
link = EZ + M,
response = exp(EZ + M + 0.5 * S)
)

@mahendra-mariadassou mahendra-mariadassou force-pushed the 114-inconsistency-between-fitted-and-predict branch from 46db73e to 4a68804 Compare January 8, 2024 08:05
Copy link
Member

@jchiquet jchiquet left a comment

Choose a reason for hiding this comment

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

alright then !

I am preparing a new CRAN submission based on master branch

@jchiquet jchiquet merged commit 3ac3d91 into master Jan 8, 2024
7 checks passed
@mahendra-mariadassou mahendra-mariadassou deleted the 114-inconsistency-between-fitted-and-predict branch January 23, 2024 15:00
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.

Inconsistency / clarification required between field fitted and function predict
2 participants