Add missing parameters to nlos-z.yaml and refactor tal.render.render module
#4
Conversation
There was a problem hiding this comment.
Wow, thanks a lot for the PR. The tal.render.render module was just one huge function and now it feels much better. I'll need to use tal these days so I'll be able to check if the render function works with different capture types (e.g. confocal or exhaustive). I'll merge the PR when I check that it also works well for these cases.
I added some comments/curiosities of mine, overall I like the result. Thanks again for the work :D
tal/render/render.py
Outdated
| # NOTE: Windows does not support multiprocessing | ||
| run_mitsuba_f() | ||
| else: | ||
| # HACK: on macOS "spawn" method, which is the default since 3.8, |
There was a problem hiding this comment.
AFAIK you are the first macOS user of tal so it's nice to know that it works. Windows gave some problems to other guys in our lab
There was a problem hiding this comment.
And do you remember, by any chance, what those problems were? If it's the same issue, maybe it's better to make a proper fix then?..
|
|
||
|
|
||
| @dataclass | ||
| class Point2D: |
There was a problem hiding this comment.
Just out of curiosity: is there any special advantage of using Point2D instead of just a tuple, apart from the nicer class name?
Seems fine for me, I'm just asking in case this would be useful in other parts of my projects
There was a problem hiding this comment.
Yeah, this is a bit controversial, initially I made it into a named tuple along the lines of
Point2D = namedtuple('Point2D', 'x y')
But then got an impression that it would be convenient to have numpy conversion methods directly on instances. But it doesn't seem to be the case, so it's probably better to revert to named tuples since they
- would provide the convenience accessors like
pt.x, - be compatible with the regular tuples, and
- require less code (literally a line per type :)
Co-authored-by: Diego Royo <diego.roymen@gmail.com>
…id 'context has already been set' exception
Co-authored-by: Diego Royo <diego.roymen@gmail.com>
|
Thanks a lot for the PR. Recently I had to make some changes to |
Hi! Please note it is more of a draft/request for comments than a polished change. E.g. it was only tested with
scan_type == "single", so likely there are problems in other regimes