nerfstudio icon indicating copy to clipboard operation
nerfstudio copied to clipboard

TensoRF fixes and improvements

Open terrancewang opened this issue 2 years ago • 0 comments

#Adding Brent's comments from the previous TensoRF PR to this issue.

  • for debugging performance issues probably good to disable mixed precision? (the original tensorf implementation also does everything in float32 right) (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960099705)

  • FYI the original implementation does reset at every step by default (you can grep for lr_upsample_reset) (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960101390)

  • should num_components be configurable? 96 also seems pretty high (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960119926)

  • is having one position_encoding decomposition and one direction_encoding decomposition consistent with the original implementation? the original TensoRF implementation uses completely separate decompositions for density + color, right? this seems different (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960120370)

  • are you sure it makes sense to use a standard NeRFField() here? see note above on separation of decompositions; TensoRF also doesn't have separate "base" and "head" MLPs right, which this will create (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960121914)

  • naming nit: there's no fine field right? so probably can drop "coarse" here (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960122586)

  • nit: feature_loss seems a bit ambiguous, maybe something like decomposition_l1_weight? (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960124188)

  • same note as above on feature_loss naming, feels a bit ambiguous (https://github.com/plenoptix/nerfactory/pull/218#discussion_r960126905)

  • https://github.com/plenoptix/nerfactory/pull/218#discussion_r960127886

terrancewang avatar Sep 01 '22 06:09 terrancewang