conditional-flow-matching
conditional-flow-matching copied to clipboard
Road to 1.1
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
I totally agree. I'd suggest the following:
- Upgrade to torch 2 first.
- Handle the tests next.
- 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.
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.
Ok. I agree with the two of you. @atong01, I was thinking about it. What about the specific requirement.txt within runner?
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 @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
@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.
- I agree
torchcfm.models
should becomeexamples.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 submodulesspaces
,plotting
, sde`, ... if you think it saves your users time.
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
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).
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: @.***>
Note about the 1.0.4 release (7-10 days):
- Release better Cifar 10 code (large and small models for FID 5-6)
- Release Intro to Flow Matching notebook
- Release Single cell notebook with conditional generation from a given sample (SB-CFM vs SF2M)
- 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?
Happy to have a discussion -- also happy to do any code reviews or issues, just tag me :)
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.