add comment in `conditional_mnist.ipynb` for training FM

atong01 / conditional-flow-matching

TorchCFM: a Conditional Flow Matching library

https://arxiv.org/abs/2302.00482

MIT License

1.25k stars 101 forks source link

add comment in `conditional_mnist.ipynb` for training FM #97

Closed ImahnShekhzadeh closed 10 months ago

ImahnShekhzadeh commented 10 months ago

This PR includes a "flag" USE_ICFM in conditional_mnist.ipynb (similar to what is done here in train_cifar10.py) to allow training both FM and ICFM.

[y] Did you make sure title is self-explanatory and the description concisely explains the PR?
[y] Did you make sure your PR does only one thing, instead of bundling different changes together?
[ ] Did you list all the breaking changes introduced by this pull request?
[y] Did you test your PR locally with pytest command?
[y] Did you run pre-commit hooks with pre-commit run -a command?

review-notebook-app[bot] commented 10 months ago

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (3f9d813) 35.92% compared to head (399ec7b) 35.92%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #97 +/- ## ======================================= Coverage 35.92% 35.92% ======================================= Files 67 67 Lines 7399 7399 ======================================= Hits 2658 2658 Misses 4741 4741 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kilianFatras commented 10 months ago

Hello,

Thank you for your contribution. We have discussed with Alex and we think it is not the purpose of notebooks to have this kind of flags and several variants. The reason is that the notebooks have a pedagogical purpose. Instead of adding a flag, it would be better in our opinion to add the following sentence under line 10 of the current notebook:

# users can try target FM by changing the above line by # FM = TargetConditionalFlowMatcher(sigma=sigma)

You are welcome to update your PR if you want to keep contributing. Once again, thank you for your contribution.

ImahnShekhzadeh commented 10 months ago

Thank you for the comment, I updated the PR.

kilianFatras commented 10 months ago

Hi,

I think it is better if we explain what we want to do to users. Therefore, I would keep

# Users can try target FM by changing the above line by # FM = TargetConditionalFlowMatcher(sigma=sigma)

Thanks

ImahnShekhzadeh commented 10 months ago

Sure!

kilianFatras commented 10 months ago

Thanks, it looks good to me. I am now merging.