Skip to content

Conversation

@wstangfi
Copy link
Collaborator

@wstangfi wstangfi commented May 29, 2025

Mainly changes on Centralize file handling & reorganize run_likelihood by Jeff Soules in the private repo

also replaces #29

Copy link
Collaborator

@jsoules jsoules left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for pushing this, and for checking in for my review.

I don't think it's necessary for me to repeat a line-by-line review of all the changes that are coming over from upstream which were already accepted into EMPM. However, I have reviewed the other PRs into the two big recent feature branches (the generate-from-trajectory branch and the benchmark). I opened secondary PRs with some edits, and also have some questions that we should discuss further in those individual PRs. (The benchmark changes, in particular, will also need to be ported back upstream into EMPM.)

For this branch, I have confirmed that the tests are all still passing (with some xfails and warnings that we should eventually address) under this branch, and I've run almost all the examples successfully. (I am encountering out-of-memory errors in the run_likelihood example; while there's probably something we should investigate in the code, I think the bigger issue is a lot of my GPU memory being reserved by some hanging process or something).

Is there anything else specifically that you'd like me to look at? Otherwise, I think once the benchmarking and trajectory stuff are stable, we should rerun the tests and examples and then this PR should be good to merge.

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