[yt-dev] Volume Rendering API is very complicated when doing non-default things

Matthew Turk matthewturk at gmail.com
Thu Jul 7 13:58:44 PDT 2016


On Thu, Jul 7, 2016 at 3:15 PM, Nathan Goldbaum <nathan12343 at gmail.com> wrote:
>
>
>
> On Thu, Jul 7, 2016 at 12:20 PM, Matthew Turk <matthewturk at gmail.com> wrote:
>>
>> Hi,
>>
>> I think getting rid of it is a good plan.
>>
>> I too have been frustrated and confused at times with the new API, but I thought it was just me not being totally up on it.  I think it opens up so many new opportunities and possibilities, and I absolutely don't want to be misunderstood that I'm a huge proponent of the new stuff, but I think there is a level of smoothing-out we could do to make things simpler.
>>
>> A few things off hand:
>>
>>  * Right now it's very difficult to get performance out of the off-axis projections that is reasonable, because of the way the VR handles kdTree construction and vertex-centering.  With getting rid of auto, we could fix this.  (OAPs don't need either!)
>>  * The utilities.py file has a lot of boilerplate code I'm not a huge fan of, that I think could be stuck elsewhere.
>>  * TransferFunctionHelper is marvelous, but maybe creates another level of indirection we don't want.
>>
>> Anyway, I think Nathan has a plan, and I'm going to also spend time this afternoon digging in to see if I can find any obvious places with minimal impact to improve things.
>
>
> Elaborating a bit more:
>
> Right now my plan is to remove the auto keyword argument and convert the VolumeSource.volume and VolumeSource.transfer_function attributes to be properties. This means that the volume (i.e. the KDTree) and the transfer function will be generated lazily. Their attributes will be generated based on sane defaults, but the user will be able to override them *before* the KDTree and transfer function actually get created for the first time.
>
> I have this working for the transfer_function and am about to start working on the volume. Part of this will be making it so the VolumeSource tracks whether or not the volume rendering should be done in log space or not.
>
> I think this will fix most of the issues Matt describes above.


I think so, yes.

After I started to dig in, and seeing how some of the classes were set
up, I started digging into the Cython code to try to make it a bit
easier to extend, and I ended up with this PR:

https://bitbucket.org/yt_analysis/yt/pull-requests/2264

>
>
>>
>>
>> One possible thing to consider is delaying a release of 3.3 until we are happy with the API, and in the process, undoing the feature freeze.  I'm kind of torn on this.
>>
>> -Matt
>>
>> On Wed, Jul 6, 2016 at 5:09 PM, Andrew Myers <atmyers2 at gmail.com> wrote:
>>>
>>> What do people think about getting rid of the auto keyword altogether? It seems like we could postpone actually building the AMRKDTree until it is needed (like, when show() is called). All the properties needed to do so (fields, logging options) can be controlled through the VolumeSource, with sensible defaults.
>>>
>>>
>>> On Wed, Jul 6, 2016 at 2:20 PM, Nathan Goldbaum <nathan12343 at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Jul 6, 2016 at 4:15 PM, Cameron Hummels <chummels at gmail.com> wrote:
>>>>>
>>>>> I agree that this is an issue.  I think the easiest way forward is just to provide an example or two where we don't use auto=True, and show what needs to be done in the instance where it isn't done automatically.  I don't think it necessitates an overhaul, but perhaps others disagree.  Since 3.3 will be the first release with the new VR interface, I think this would be appropriate to include before the release.
>>>>
>>>>
>>>> I think more documentation is part of the solution, but I also think there's low-hanging fruit to improve the API.
>>>>
>>>> For example, I don't think users should ever need to manually create an AMRKDTree object. I should be able to control the creation of the volume from the VolumeSource object itself.
>>>>
>>>>>
>>>>>
>>>>> Cameron
>>>>>
>>>>> On Wed, Jul 6, 2016 at 1:57 PM, Nathan Goldbaum <nathan12343 at gmail.com> wrote:
>>>>>>
>>>>>> Hi all,
>>>>>>
>>>>>> tl;dr:
>>>>>>
>>>>>> I have a high-level issue with the volume rendering API. I think there are some simple things that can be done to make it simpler to use, but I'm not sure how much work it will take to get us there.
>>>>>>
>>>>>> ----------------------------------------------------
>>>>>>
>>>>>> Today I was looking at issue #1234: https://bitbucket.org/yt_analysis/yt/issues/1234/volume-rendering-example-in-loading
>>>>>>
>>>>>> This refers to an example in our documentation that uses the old volume rendering API to make a very pretty image. I wanted to retain that image, which led me to try to make a fully customized volume rendering using the new interface for the first time, and it left me very confused. If I was completely unfamiliar with yt internals and not comfortable looking at the yt source code, I don't think I would have been able to successfully recreate the image using the new API.
>>>>>>
>>>>>> To make that concrete, here's how you would construct the volume rendering using the old interface:
>>>>>>
>>>>>> https://bpaste.net/show/58194a5a1352
>>>>>>
>>>>>> And here's what I did to make the same image using the new volume rendering interface:
>>>>>>
>>>>>> https://bpaste.net/show/9fba5872e532
>>>>>>
>>>>>> Three lines of code in the old interface expand into 26 lines of code in the new interface. I also need to do scary-looking undocumented things like manually creating an AMRKDTree to be able to get the field logging setting I wanted. I also need to know about what's going on under the hood in the VolumeSource class since I wanted to avoid setting auto=True, which does a lot of basic setup, but also does some unnecessary computation.
>>>>>>
>>>>>> That said, some of the additional boilerplate constitutes an improvement in clarity. For example, I like how I'm setting camera attributes now instead of setting keyword arguments in the Camera initializer.
>>>>>>
>>>>>> The main problem I see is that the VR interface attempts to do too much "magic" by default. If auto=True is set when the VolumeSource is created, an AMRKDTree is constructed, fields to render are set, whether or not the rendering happens in log space is determined by looking at the take_log setting in the FieldInfo, and a transfer function is created based on a TransferFunctionHelper instance. If auto=False, it's not clear about how one sets this stuff up without looking carefully at what happens inside the VolumeSource code when auto=True.
>>>>>>
>>>>>> It would be nice if there were a better story in the auto=False universe. For example, the VolumeSource object could grow a way to *automatically* create AMRKDtree instances when needed, after a user sets the field to render and sets which fields should be rendered in log space. This is almost possible in the current API, but the VolumeSource object would need to grow an API that allows the user to control the field logging behavior - right now this is only controllable at the level of the AMRKDTree object.
>>>>>>
>>>>>> I'm curious whether anyone has suggestions or ideas for ways forward on this. Is this a documentation problem (i.e. do we just need to add a ton of examples where we do customized volume renderings in a somewhat baroque but very explicitt style) or indicative of deeper issues with the new VR interface? Should the issues I'm raising be dealt with before we release yt 3.3 or could we patch it afterwards?
>>>>>>
>>>>>> All that said, I don't particularly want to embark on a several weeks-long yak-shaving expedition in the volume rendering code and I can't volunteer to lead any kind of overhaul. I am willing to make small usability improvements, though, if that's what we decide we should do.
>>>>>>
>>>>>> Hope that wasn't too much of a novel, I'd love to hear comments about this.
>>>>>>
>>>>>> -Nathan
>>>>>>
>>>>>> _______________________________________________
>>>>>> yt-dev mailing list
>>>>>> yt-dev at lists.spacepope.org
>>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Cameron Hummels
>>>>> NSF Postdoctoral Fellow
>>>>> Department of Astronomy
>>>>> California Institute of Technology
>>>>> http://chummels.org
>>>>>
>>>>> _______________________________________________
>>>>> yt-dev mailing list
>>>>> yt-dev at lists.spacepope.org
>>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> yt-dev mailing list
>>>> yt-dev at lists.spacepope.org
>>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>>
>>>
>>>
>>> _______________________________________________
>>> yt-dev mailing list
>>> yt-dev at lists.spacepope.org
>>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>>
>>
>>
>> _______________________________________________
>> yt-dev mailing list
>> yt-dev at lists.spacepope.org
>> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>>
>
>
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>



More information about the yt-dev mailing list