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

add comment in conditional_mnist.ipynb for training FM #97

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

ImahnShekhzadeh
Copy link
Contributor

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?

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #97   +/-   ##
=======================================
  Coverage   35.92%   35.92%           
=======================================
  Files          67       67           
  Lines        7399     7399           
=======================================
  Hits         2658     2658           
  Misses       4741     4741           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kilianFatras
Copy link
Collaborator

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 ImahnShekhzadeh changed the title add flag USE_ICFM in conditional_mnist.ipynb to allow training both FM and ICFM add comment in conditional_mnist.ipynb for training FM Jan 11, 2024
@ImahnShekhzadeh
Copy link
Contributor Author

Thank you for the comment, I updated the PR.

@kilianFatras
Copy link
Collaborator

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
Copy link
Contributor Author

Sure!

@kilianFatras
Copy link
Collaborator

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

@kilianFatras kilianFatras merged commit 7898ff1 into atong01:main Jan 15, 2024
33 checks passed
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.

2 participants