search

PLN-team / PLNmodels

A collection of Poisson lognormal models for multivariate count data analysis
https://pln-team.github.io/PLNmodels
GNU General Public License v3.0
54 stars 18 forks source link

Changes from Cole Trapnell #110

Closed brgew closed 10 months ago

brgew commented 11 months ago

There are essentially three groups of changes (in order of most important to least important for our lab):

  1. The computation and storage of the variance-covariance matrix on model parameters (theta) when using the bootstrap.
  2. An expanded interface for configuring the model initialization, to allow for customization of the "post-treatment" steps.
  3. Changes to the torch optimizer to smooth the way for running on GPUs
ctrapnell commented 10 months ago

@mahendra-mariadassou thanks for the question. I replied to this question in the wrong place yesterday - but I think the answer is that the use case for covariance = "spherical" or "diagonal" in PLNnetwork is when you have fewer samples than you have species. IIRC, the issue was the call to solve() in "fixed" optimize() function. If n < p, that solve can throw an exception, never making it to the sparse estimation (which can deal with the n < p case at least insofar as returning an answer).

mahendra-mariadassou commented 10 months ago

Hi,

We made another pass with @jchiquet and made a few suggestions on a new tweaks branch. Since I'm not a github expert, I branched tweaks from PLN-team:master, pulled cole-trapnell-lab:master into and then pushed a few commits (so that it has our suggestions on top of your PR).

Here are the suggestions:

You can pull PLN-team:tweaks into your master and make additional changes if you want, this should automatically update this PR.

jchiquet commented 10 months ago

To complete @mahendra-mariadassou 's answer, regarding lapply vs future_lapply, I suggest, as mahendra said, that we open a specific issue since it concerns all paralelissable action. Like Cole, I had noted that using future with a multi-threaded BLAS/LAPACK library (such as OpenBLAS) could have detrimental consequences. I therefore suggest defining a lapply function which, depending on the architecture in place, directs towards a classic or multicore lapply. See if future is capable of this ('sequential' or 'multicore' plan).

This is now referenced as Issue https://github.com/PLN-team/PLNmodels/issues/111

ctrapnell commented 10 months ago

Hi PLN-team. Thanks for creating this. We have pulled this branch into our fork, merged in the changes from the main fork's master, and run our tests. All looks great to us!

mahendra-mariadassou commented 10 months ago

Hi @ctrapnell. You probably received an automatic email from github, but since I didn't use a standard workflow and just in case. I merged all your changes (amended with our changes in PLNteam:tweaks) into master, which automatically closed the PR.

Thanks again for the PR, the various changes and more generally your interest in PLNmodels !

ctrapnell commented 10 months ago

Terrific, thank you!

C

On Tue, Nov 28, 2023 at 11:29 AM Mahendra Mariadassou < @.***> wrote:

Hi @ctrapnell https://urldefense.com/v3/__https://github.com/ctrapnell__;!!K-Hz7m0Vt54!njTuq46c0WKSBLZaowyaAoXcvyIUuedPJIFDfV5hC8BB9HpqqzwpbqtGDzu9BEu8-rOBTE25g8lXNWJb5Jgb8NE$. You probably received an automatic email from github, but since I didn't use a standard workflow and just in case. I merged all your changes (amended with our changes in PLNteam:tweaks) into master, which automatically closed the PR.

Thanks again for the PR, the various changes and more generally your interest in PLNmodels !

— Reply to this email directly, view it on GitHub https://urldefense.com/v3/__https://github.com/PLN-team/PLNmodels/pull/110*issuecomment-1830537129__;Iw!!K-Hz7m0Vt54!njTuq46c0WKSBLZaowyaAoXcvyIUuedPJIFDfV5hC8BB9HpqqzwpbqtGDzu9BEu8-rOBTE25g8lXNWJbTyBEiPQ$, or unsubscribe https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AACH6GPNXV6G3YH5NJKYPQDYGY3QRAVCNFSM6AAAAAA6R4PYSOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZQGUZTOMJSHE__;!!K-Hz7m0Vt54!njTuq46c0WKSBLZaowyaAoXcvyIUuedPJIFDfV5hC8BB9HpqqzwpbqtGDzu9BEu8-rOBTE25g8lXNWJbkZp9R2g$ . You are receiving this because you were mentioned.Message ID: @.***>

jchiquet commented 10 months ago

Thank you all very much. I'm preparing a version for CRAN today, now that Mahendra has done most of the work.

jchiquet commented 10 months ago

@ctrapnell, @maddyduran and @brgew we'd like to add you to the list of package contributors, do you agree? If so, I'll need an e-mail address for each of you (I get coletrap@uw.edu, duran@uw.edu and bge@uw.edu, is this correct?)

brgew commented 10 months ago

Hi Julien,

I made no contribution to the pull request. I merely posted it. So I prefer to not be added to the list of contributors. But thank you anyway!

Ever grateful, Brent

On Wed, Nov 29, 2023 at 1:01 AM Julien Chiquet @.***> wrote:

@ctrapnell https://urldefense.com/v3/__https://github.com/ctrapnell__;!!K-Hz7m0Vt54!nv5Aci4H9eHz65kV-DCuxmUnXm-kZhJzK2Of2eJO96JsPXredA3R-6qUhtI9Q-KHdpQIb8YP2_9FSSVeQ-yI$, @maddyduran https://urldefense.com/v3/__https://github.com/maddyduran__;!!K-Hz7m0Vt54!nv5Aci4H9eHz65kV-DCuxmUnXm-kZhJzK2Of2eJO96JsPXredA3R-6qUhtI9Q-KHdpQIb8YP2_9FScT_QQyL$ and @brgew https://urldefense.com/v3/__https://github.com/brgew__;!!K-Hz7m0Vt54!nv5Aci4H9eHz65kV-DCuxmUnXm-kZhJzK2Of2eJO96JsPXredA3R-6qUhtI9Q-KHdpQIb8YP2_9FSevjGdXk$ we'd like to add you to the list of package contributors, do you agree? If so, I'll need an e-mail address for each of you (I get @., @. and @.***, is this correct?)

— Reply to this email directly, view it on GitHub https://urldefense.com/v3/__https://github.com/PLN-team/PLNmodels/pull/110*issuecomment-1831485538__;Iw!!K-Hz7m0Vt54!nv5Aci4H9eHz65kV-DCuxmUnXm-kZhJzK2Of2eJO96JsPXredA3R-6qUhtI9Q-KHdpQIb8YP2_9FScc8TJUR$, or unsubscribe https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ACXSPQ5H3UNG5NY6ZL3JQEDYG32XHAVCNFSM6AAAAAA6R4PYSOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRGQ4DKNJTHA__;!!K-Hz7m0Vt54!nv5Aci4H9eHz65kV-DCuxmUnXm-kZhJzK2Of2eJO96JsPXredA3R-6qUhtI9Q-KHdpQIb8YP2_9FScO6sAF4$ . You are receiving this because you were mentioned.Message ID: @.***>

jchiquet commented 10 months ago

Thank you @brgew for your answer! I assume then that @maddyduran and @ctrapnell, who actually contributed to the PR, would like to be added as contributors. Best

ctrapnell commented 10 months ago

Hi Julien,

Thank you for thinking of adding us - not sure that's needed given our modest contribution, but we are grateful for being asked. Either way is OK, depending on what you generally do for the project with PRs. Thanks again for such a great tool and being open to contributions.

Best,

Cole

On Fri, Dec 1, 2023 at 10:58 PM Julien Chiquet @.***> wrote:

Thank you @brgew https://urldefense.com/v3/__https://github.com/brgew__;!!K-Hz7m0Vt54!nwCz7GUTnHBAEmEVKBcK4AOBmleYuuGSw0kiuSYYbaHDyN7bX1_eBhSrznLLkAhRhJExh0o21DnhOxvI8sHJy-s$ for your answer! I assume then that @maddyduran https://urldefense.com/v3/__https://github.com/maddyduran__;!!K-Hz7m0Vt54!nwCz7GUTnHBAEmEVKBcK4AOBmleYuuGSw0kiuSYYbaHDyN7bX1_eBhSrznLLkAhRhJExh0o21DnhOxvI9FQ_LG4$ and @ctrapnell https://urldefense.com/v3/__https://github.com/ctrapnell__;!!K-Hz7m0Vt54!nwCz7GUTnHBAEmEVKBcK4AOBmleYuuGSw0kiuSYYbaHDyN7bX1_eBhSrznLLkAhRhJExh0o21DnhOxvI32LJMVc$, who actually contributed to the PR, would like to be added as contributors. Best

— Reply to this email directly, view it on GitHub https://urldefense.com/v3/__https://github.com/PLN-team/PLNmodels/pull/110*issuecomment-1837064929__;Iw!!K-Hz7m0Vt54!nwCz7GUTnHBAEmEVKBcK4AOBmleYuuGSw0kiuSYYbaHDyN7bX1_eBhSrznLLkAhRhJExh0o21DnhOxvI_phU7gk$, or unsubscribe https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AACH6GLTA44RTLUKJ4FZWWTYHLGRBAVCNFSM6AAAAAA6R4PYSOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZXGA3DIOJSHE__;!!K-Hz7m0Vt54!nwCz7GUTnHBAEmEVKBcK4AOBmleYuuGSw0kiuSYYbaHDyN7bX1_eBhSrznLLkAhRhJExh0o21DnhOxvIzBypWGw$ . You are receiving this because you were mentioned.Message ID: @.***>