conditional-flow-matching icon indicating copy to clipboard operation
conditional-flow-matching copied to clipboard

Road to 1.1

Open kilianFatras opened this issue 1 year ago • 13 comments

I am opening this issue to discuss the road we should follow to the 1.1 version. Here is the list of the changes I would like to see:

  • [x] Add Cifar10 experiments
  • [x] Use torchdiffeq and remove torchdyn in examples (solved with torchdyn 1.0.6)
  • [x] Upgrade to Torch 2.0 (and update requirements) (solved with torchdyn 1.0.6)
  • [x] Add conditional examples
  • [x] Add Tests
  • [x] Add Forest-Flow example
  • [ ] Add train single cell files
  • [x] Add tutorials
  • [ ] Refactoring (utils + Models)
  • [ ] Add SO(3) example

kilianFatras avatar Aug 12 '23 20:08 kilianFatras

I totally agree. I'd suggest the following:

  1. Upgrade to torch 2 first.
  2. Handle the tests next.
  3. Add the conditional examples.

If you do this in 3 sequential PRs they will be less nasty and confusing.

The change from torchdiffeq is because the torchdyn folks refuse to maintain their code?

Makes sense. But I think I would do this change in parallel with the move to pytorch 2, because you might discover nasty bugs upgrading to pytorch 2 and realizing those packages don't play nice with it.

josephdviviano avatar Aug 14 '23 06:08 josephdviviano

I believe the basic torchcfm codebase plays fairly nicely with torch 2.0, but the code in runner reproducing things from the papers does not. Specifically even the versions of pytorch_lightning and even torch_metrics seem to break existing tests in the runner code. I think we should probably keep the fully reproducible paper code requiring torch < 2.0, but make nicer code in torch 2.0.

atong01 avatar Aug 14 '23 06:08 atong01

Ok. I agree with the two of you. @atong01, I was thinking about it. What about the specific requirement.txt within runner?

kilianFatras avatar Aug 14 '23 11:08 kilianFatras

you could indeed have two different pip installs. You could at that point also consider making runner a separate repo that imports from this library if it's main purpose is archival (one could imagine pulling over a few examples that are torch2 compatible into the main library as a set of explanatory examples).

josephdviviano avatar Aug 15 '23 16:08 josephdviviano

@josephdviviano @atong01 I think the priority is to make a clean cifar10 folder. I am already working on it and hope to submit PR by the end of the week. https://github.com/kilianFatras/conditional-flow-matching/tree/cifar10_experiments

kilianFatras avatar Aug 16 '23 00:08 kilianFatras

@atong01 @josephdviviano I have been thinking about the current structure of the library. I would like to change some elements and request your opinions.

  • The current folder 'models' within the torchcfm folder should either be moved to the main folder or within examples. It does not make a lot sense to have where it is now.
  • The current file 'utils' within the torchcfm folder should be moved to the main folder. It does not make a lot of sense to have where it is now. In addition, it should become a folder as we will have a lot of new utils elements not related to Euclidean space in the coming months. We could also put everything related to drawing toy datasets and SDE/ODE elements in different files.

I think it would make sense to do that in 1.1 but it could also be in 1.2. Let me know your thoughts on that.

kilianFatras avatar Aug 19 '23 15:08 kilianFatras

  • I agree torchcfm.models should become examples.models.
  • If the utils files are useful for handling different spaces maybe they should stay in the main library?
  • You could have a module utils with submodules spaces, plotting, sde`, ... if you think it saves your users time.

josephdviviano avatar Aug 19 '23 18:08 josephdviviano

I noticed that torchdyn has released v1.0.6 in case that changes anything for you

Also, thanks for the very nice and easy to read code! :D

Edit: Tried v1.0.6 and it works for me but I'm not running your full examples

samedii avatar Sep 06 '23 22:09 samedii

Hi @samedii, thank you for your interest in our TorchCFM library! Yes, we are aware of torchdyn 1.0.6 and that it solves some of our issues :).

@atong01 @josephdviviano let's discuss which tests should be added to the library. With torchdyn 1.0.6 everything is working now (note that I will recompute all examples next week with torch2.0 to be 100% sure). Also, the conditional_examples are ready (pep8 to finalize before merging).

kilianFatras avatar Sep 10 '23 16:09 kilianFatras

Just FYI this week for me is a bit crazy, let's book a slot next week to discuss :) Joseph Viviano @josephdviviano https://twitter.com/josephdviviano viviano.ca

On Sun, Sep 10, 2023 at 12:55 PM Kilian @.***> wrote:

Hi @samedii https://github.com/samedii, thank you for your interest in our TorchCFM library! Yes, we are aware of torchdyn 1.0.6 and that it solves some of our issues :).

@atong01 https://github.com/atong01 @josephdviviano https://github.com/josephdviviano let's discuss which tests should be added to the library. With torchdyn 1.0.6 everything is working now (note that I will recompute all examples next week with torch2.0 to be 100% sure). Also, the conditional_examples are ready (pep8 to finalize before merging).

— Reply to this email directly, view it on GitHub https://github.com/atong01/conditional-flow-matching/issues/37#issuecomment-1712873141, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7TL2X3XYD2KB5KCTKQNZ3XZXWGLANCNFSM6AAAAAA3OEPXNI . You are receiving this because you were mentioned.Message ID: @.***>

josephdviviano avatar Sep 12 '23 21:09 josephdviviano

Note about the 1.0.4 release (7-10 days):

  1. Release better Cifar 10 code (large and small models for FID 5-6)
  2. Release Intro to Flow Matching notebook
  3. Release Single cell notebook with conditional generation from a given sample (SB-CFM vs SF2M)
  4. Notebook showing the mb influence on inference

I still do not have an idea about what tests we should have. let's have a discussion all together after the 1.0.4 release.

@atong01 anything else to add?

kilianFatras avatar Oct 14 '23 01:10 kilianFatras

Happy to have a discussion -- also happy to do any code reviews or issues, just tag me :)

josephdviviano avatar Oct 16 '23 16:10 josephdviviano

The PR on Forest-Flow is ready to be merged and it is waiting for approval from @atong01. It also adds a test on the time vector 't'.

I have opened a PR on tests. Note that this is a first draft that ensures that the main classes are working. I will add in the future tests on the OT classes as well.

kilianFatras avatar Nov 14 '23 20:11 kilianFatras