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

Road to 1.1 #37

Open
7 of 10 tasks
kilianFatras opened this issue Aug 12, 2023 · 13 comments · Fixed by #40
Open
7 of 10 tasks

Road to 1.1 #37

kilianFatras opened this issue Aug 12, 2023 · 13 comments · Fixed by #40
Assignees
Labels
enhancement New feature or request

Comments

@kilianFatras
Copy link
Collaborator

kilianFatras commented Aug 12, 2023

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:

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

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.

@atong01
Copy link
Owner

atong01 commented Aug 14, 2023

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.

@kilianFatras
Copy link
Collaborator Author

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

@josephdviviano
Copy link
Collaborator

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).

@kilianFatras
Copy link
Collaborator Author

@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 kilianFatras linked a pull request Aug 17, 2023 that will close this issue
5 tasks
@kilianFatras
Copy link
Collaborator Author

kilianFatras commented Aug 19, 2023

@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.

@josephdviviano
Copy link
Collaborator

  • 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.

@samedii
Copy link

samedii commented Sep 6, 2023

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

@kilianFatras
Copy link
Collaborator Author

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).

@josephdviviano
Copy link
Collaborator

josephdviviano commented Sep 12, 2023 via email

@kilianFatras
Copy link
Collaborator Author

kilianFatras commented Oct 14, 2023

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?

@josephdviviano
Copy link
Collaborator

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

@kilianFatras
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants