Skip to content

Conversation

psteinb
Copy link
Collaborator

@psteinb psteinb commented Mar 18, 2025

  • created noise_schedule method to be overwritten by derivatives
  • created times_schedules method to be overwritten by derivatives
  • created test on times_schedule
  • improvded docstrings
  • created test on noise_schedule

This addresses #1437

@psteinb psteinb force-pushed the psteinb-explicit_noise_schedules-1437 branch from 966a44c to 45b1e94 Compare March 19, 2025 10:58
@psteinb psteinb requested a review from manuelgloeckler March 20, 2025 17:21
@psteinb
Copy link
Collaborator Author

psteinb commented Mar 20, 2025

@manuelgloeckler So i've finished working on this.

  • times_schedule and noise_schedule are now member functions of the ConditionalScoreEstimator base class
  • I tried my luck in setting up something like the EDM paper [1], but failed. I found it too hard to map the current API to what is described in [1]
  • Taking this into account, if we like to have something like this, we would have to implement the complete forward pass as well as the training loop perhaps (perhaps in the wake of Better training loop for diffusion models and flow matching #1445)

If the commit history scares you, either squash them all or let me know. I can also close this draft and send another PR.

[1] https://arxiv.org/abs/2206.00364

psteinb added 18 commits March 20, 2025 18:38
- created noise_schedule method to be overwritten by derivatives
- created times_schedules method to be overwritten by derivatives
- created test on times_schedules
- improvded docstrings
in addition:
- added improved version to benchmarks (for later comparison)
- created new class ImprovedVPScoreEstimator
to understand how the VE estimator is implemented
- without touching the forward function of ConditionalScoreEstimator
- benchmarks show that this leads to very long training time
without any performance improvements
@psteinb psteinb force-pushed the psteinb-explicit_noise_schedules-1437 branch from 5c0798a to 833c17b Compare March 20, 2025 17:38
@psteinb psteinb marked this pull request as ready for review March 21, 2025 08:39
@StarostinV
Copy link
Collaborator

It's great to have it fixed! This PR will have merge conflicts with #1497 , so I decided to take a look. I have a question - as far as I understand, during inference the introduced time schedule is still ignored and the linear steps are generated here in score_posterior.py, is that correct? I am wondering whether it should be handled by the Diffuser object instead of the posterior. Currently, Diffuser.run just receives ts, but I think it would be great to save time_scheduler along with t_min and t_max in the constructor and then construct ts based on the provided num_steps.

@manuelgloeckler manuelgloeckler added the blocked Something is in the way of fixing this. Refer to it in the issue label Mar 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Something is in the way of fixing this. Refer to it in the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants