Skip to content

Omit unneeded marginalization nodes and rework messaging for setupMargNodes warnings#1582

Merged
paciorek merged 4 commits into
develfrom
omit_unneeded_latents
Aug 23, 2025
Merged

Omit unneeded marginalization nodes and rework messaging for setupMargNodes warnings#1582
paciorek merged 4 commits into
develfrom
omit_unneeded_latents

Conversation

@paciorek
Copy link
Copy Markdown
Contributor

@paciorek paciorek commented Aug 1, 2025

Per nimbleQuad issue 56, this (by default but overrideable by a nimble opton) omits extraneous latent nodes that a user might have included in randomEffectsNodes or latentNodes that are passed into setupMargNodes. If this is not done, Laplace inner optimization can fail if a latent node in its own isolated Laplace approximation has no data dependents.

I've added a simple test. It would be helpful if @perrydv takes a look at this, in particular the test to see if this is the behavior we want and if there are more complicated situations we should test.

@perrydv
Copy link
Copy Markdown
Contributor

perrydv commented Aug 17, 2025

@paciorek this looks good to me. Just a few minor comments. This is purely coding style, but I don't use ifelse for a non-vectorized case, but anyway that's minutiae and your choice. I think when checking nimble options it can be slightly safer to enclose in isTRUE (or sometimes one might prefer !isFALSE), so that if some weird value (or NULL) gets put into the option it won't break runs. I am not sure if using the term "marginalization" in the revised messages will be clearer for all users, and/or if it will be clear that we are referring to the intended action of the algorithm one is requesting. And then in the case of MCEM, there is not really marginalization but it still (I think) calls setupMargNodes because the graph handling is all the same. I don't want to get tied up on word-smithing but just noting those thoughts for you to consider when you finalize the messages as you choose.

@paciorek
Copy link
Copy Markdown
Contributor Author

Thanks for the suggestions - I'm following them for the most part.

I would ideally like to use the term "marginalization", but MCEM makes it more complicated - it's not exactly marginalization, but the averaging over the unknowns is similar to marginalization (and our MCEM is not much used) so maybe it's ok to use that term. But I am leaving this for another day and omitting "marginalization" from the message.

And of course, it is a bit discordant that MCEM uses a function named setupMargNodes.

@paciorek paciorek merged commit 6ee714b into devel Aug 23, 2025
0 of 8 checks passed
@paciorek paciorek deleted the omit_unneeded_latents branch August 23, 2025 20:11
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