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

86 1 multidms softplus clipping #91

Closed
wants to merge 1 commit into from

Conversation

jgallowa07
Copy link
Collaborator

@jgallowa07 jgallowa07 commented Dec 6, 2023

This PR implements the changes outlined in #86 and is running on the test_example data on my end. It is still a work in progress (and thus draft PR) until @Caleb-Carr and I get a chance to review the results of this branch on some data other than the test_example. After that we will outline thoughts, bugs, and feedback and re-iterate for a merga-able PR submission.

Notes to expand upon:

  • This updates fixes a logical bug in the func effect shifts notebook - this means that folks results will inevitably be different.
  • A small ridge penalty on the beta's keeps the GE fits more stable - exposed a set of kwargs for multidms fitting in func effect config
  • The output format of the averaged func effects notebook has changes as we're using the new interface. This still needs to be clarified
  • The output of the individual shifts inference notebook now includes the pickle file encoding the sweep of fits across lasso shift coefficients
  • Could potentially add more heatmaps in all notebooks for shifts, predicted func scores, and beta's alike. The infrastructure is simple.

@jgallowa07 jgallowa07 force-pushed the 86_1_multidms_softplus_clipping branch from cb89b1f to f227d7f Compare January 10, 2024 18:24
@jbloom
Copy link
Contributor

jbloom commented Jan 10, 2024

@jgallowa07, is this something I should review now, or are you still working on it?

Can you just tag me either way when it is ready for me to look at?

@jgallowa07
Copy link
Collaborator Author

@jbloom Still a work in progress. I'll convert it to a full-fledged PR and request your review when it's ready for a look over. Thanks

@jbloom
Copy link
Contributor

jbloom commented Apr 23, 2024

@jgallowa07, I was curious on the status of this pull request, and if it is a long way from merging if there is just something we can do about the issue with the current version that causes problems with newer version of pandas: matsengrp/multidms#128

@jgallowa07
Copy link
Collaborator Author

Sorry @jbloom - I should have checked in with you.

I'm guessing you have not talked with Hugh or Will about the convergence issues we're currently trying to fix by implementing a generalized fused-lasso approach? I've just validated that a prototype of this model produces very similar results on spike & simulated data, but am just now starting to hack at the actual implementation into the multidms source code. Inevitably this will require more changes the workflow here once that's finished.

So I see two options:

  1. I could get this PR working now with a version of multidms that fixes the pandas issue if you don't want to wait on the changes above. It's just a bit of duplicated effort but I'm okay with that.
  2. Wait till we've finished the above (~ 3ish weeks is my guess given the amount of changes needed), and disable the multidms workflow from this pipeline in the meantime.

Happy to do whatever you think is best. Also happy to set up a meeting if you have time and would like to discuss these changes.

@jbloom
Copy link
Contributor

jbloom commented Apr 23, 2024

If it is only ~3 weeks, that is fine for now.

@jgallowa07
Copy link
Collaborator Author

jgallowa07 commented May 8, 2024

@jbloom Update: We've settled on an approach and I'm in the process of updating multidms. I'm hoping to have it done by EOW, and then have this finished by the end of next week.

@jgallowa07
Copy link
Collaborator Author

Sorry @jbloom - I'm the only one doing the coding for multidms, and with this major refactor and balancing other projects I've fallen behind schedule a bit. I'm hopeful that I'll have a stable version by the end of this week so I can update this pipeline for you.

@jgallowa07
Copy link
Collaborator Author

multidms 1.0.0 was just released. Working on this PR now.

@jgallowa07 jgallowa07 closed this Jun 21, 2024
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