Closed JunzheJosephZhu closed 10 months ago
bennyguo
commented
11 months ago Hi Joseph, thank you for the contribution! I saw you're using stable-diffusion-guidance instead of stable-diffusion-unified-guidance for the HiFA implementation, where the latter provides useful options like return_rgb_1step_orig and return_rgb_multistep_orig which can make HiFA implementation much easier and keep a clean code structure. Could you please take a look?
JunzheJosephZhu
commented
11 months ago sounds good, will take a look asap. One question: I only see this guidance used in magic123 and experimental/dreamfusion-sd. Isn't this going to make the changes I propose harder to adapt to existing systems since the guidance needs to be switched?
Edit: I added HiFA to unified guidance. Btw, I dont think unified guidance has been tested properly before I changed it. The configs under experimental/unified doesn't run, unified guidance is returning matrices(latents, images, etc), and the system is supposed to log all values in guidance_out, but lightning doesnt allow logging, so it throws an error. I fixed this by adding a conditional check. My guess is somebody modified stable-diffusion-unified-guidance but only tested it on magic123. You should add some unit tests to ensure that whenever a guidance is changed, all configs using it still run properly.
bennyguo
commented
11 months ago @JunzheJosephZhu Sorry for the inconvenience. stable-diffusion-unified-guidance covers all the functionality of stable-diffusion-guidance and ideally should be used in all systems. Feel free to use it in HiFA system and I'll switch the guidance in other systems to the unified guidance soon.
JunzheJosephZhu
commented
11 months ago Sounds good. All my code has been pushed. Could you go through the commit and approve it please?
JunzheJosephZhu
commented
11 months ago Could we have a quick call? My wechat is 17717613901, would like to clarify a few implementation details
On Mon, Dec 18, 2023 at 20:10 Ruizhi Shao @.***> wrote:
@.**** requested changes on this pull request.
Thank you for your work and effort! However, I noticed that there are many modifications in this commit, some of which may not be necessary. We aim to keep the original system unchanged to ensure rigorous academic comparison with the original method. If there are new systems like HIFA, it would be better to create new configurations and systems rather than modifying the existing ones. In addition, modifications to the guidance should ideally be integrated into the unified guidance.
I think the most important thing is to maintain the consistency and clarity of our codebase. I appreciate your understanding and look forward to your next commit.
On configs/dreamfusion-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430014825 :
I think this modifications were not necessary.
On configs/experimental/co3d-imagecondition.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430015152 :
I think this modifications were not necessary.
On configs/experimental/imagecondition.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430015391 :
I think this modifications were not necessary.
On configs/experimental/imagecondition_zero123nerf.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430015611 :
I think this modifications were not necessary.
On configs/experimental/imagecondition_zero123nerf_refine.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430016802 :
I think this modifications were not necessary.
On configs/experimental/prolificdreamer-importance.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430016934 :
I think this modifications were not necessary.
On configs/experimental/prolificdreamer-neus-importance.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430017296 :
I think this modifications were not necessary.
On configs/experimental/prolificdreamer-propnet.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430018112 :
I think this modifications were not necessary.
On configs/experimental/unified-guidance/dreamfusion-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430018644 :
I think this modifications were not necessary.
On configs/fantasia3d-texture.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430019152 :
I think this modifications were not necessary.
On configs/fantasia3d.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430019361 :
I think this modifications were not necessary.
On configs/gradio/dreamfusion-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430019649 :
I think this modifications were not necessary.
On configs/gradio/fantasia3d.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430019821 :
I think this modifications were not necessary.
On configs/gradio/latentnerf.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430020040 :
I think this modifications were not necessary.
On configs/gradio/sjc.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430020237 :
I think this modifications were not necessary.
On configs/latentnerf-refine.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430020537 :
I think this modifications were not necessary.
On configs/latentnerf.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430020727 :
I think this modifications were not necessary.
On configs/magic123-coarse-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430020995 :
I think this modifications were not necessary.
On configs/magic123-refine-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430021220 :
I think this modifications were not necessary.
On configs/magic3d-coarse-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430021422 :
I think this modifications were not necessary.
On configs/magic3d-refine-sd.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430023217 :
I think this modifications were not necessary.
On configs/prolificdreamer-geometry.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430023484 :
I think this modifications were not necessary.
On configs/prolificdreamer-patch.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430023675 :
I think this modifications were not necessary.
On configs/prolificdreamer-scene.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430023905 :
I think this modifications were not necessary.
On configs/prolificdreamer-texture.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430024104 :
I think this modifications were not necessary.
On configs/prolificdreamer.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430024280 :
I think this modifications were not necessary.
On configs/sjc.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430024542 :
I think this modifications were not necessary.
On configs/sketchshape-refine.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430024711 :
I think this modifications were not necessary.
On configs/sketchshape.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430024930 :
I think this modifications were not necessary.
On threestudio/systems/prolificdreamer.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430026530 :
I think this modifications were not necessary.
On launch.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430029948 :
I think this modifications were not necessary. You can set system.guidance.trainer_max_steps=${trainer.max_steps} in config.
On threestudio/models/renderers/nerf_volume_renderer.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430034268 :
How about adding an option to choose z_variance or z_variance_hifa?
On threestudio/systems/magic123.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430035022 :
I think this modifications were not necessary.
On threestudio/systems/dreamfusion.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430035478 :
I think this modifications were not necessary.
On threestudio/models/guidance/stable_diffusion_guidance.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430039116 :
I think this modifications were not necessary. These changes can be merged into unified guidance.
On threestudio/models/guidance/stable_diffusion_vsd_guidance.py https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430039641 :
I think this modifications were not necessary. These changes can be merged into unified guidance.
On configs/experimental/unified-guidance/prolificdreamer.yaml https://github.com/threestudio-project/threestudio/pull/367#discussion_r1430042744 :
How about creating a new config file named prolicdreamer-hifa.yaml?
— Reply to this email directly, view it on GitHub https://github.com/threestudio-project/threestudio/pull/367#pullrequestreview-1786724397, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF2C6G34DBJWIRHTGLVOYYTYKAXC3AVCNFSM6AAAAABAXXWDVSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOBWG4ZDIMZZG4 . You are receiving this because you were mentioned.Message ID: @.***>
DSaurus
commented
11 months ago Hi @JunzheJosephZhu ,
I have invited you to join threestudio project. It would be better if you could create a branch and submit a new pull request.
Fixed z-variance loss by normalizing weights. This is crucial to ensure that the model isn't encourage to generate empty pixels(since empty pixels have zero z-variance)
Added other HiFA(image space loss, annealing) changes to stable-diffusion-vsd-guidance, and created 2 corresponding configs(for prolificdreamer and prolificdreamer-scene). All changes are option based and do not change normal behavior if use_img_loss and sqrt_annealing are both set to false in config. I had to pass the value of trainer's max_steps to system.guidance in launch script, since this is the only way to do annealing based on global steps.
Added HiFA config which is based on dreamfusion-sd. I made minor changes to dreamfusion-sd to look out for z-variance loss, and changed stable-diffusion-guidance similar to how i changed stable-diffusion-vsd-guidance.
Will do another pull request soon to update readme and link to my official repo(which was recently released). I'm still running experiments to search for the best z-variance hyperparameter in threestudio, will update that later too in that PR.