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

Sam Skillman samskillman at gmail.com
Sun Jul 10 15:40:04 PDT 2016 

All these VR related updates sound good to me. I'm not surprised that
people are finding rough edges. Let me know if I can help in some way, but
it look like folks are tackling the right bits.

On Thu, Jul 7, 2016 at 2:59 PM Matthew Turk <matthewturk at gmail.com> wrote:

> 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
> >
> _______________________________________________
> yt-dev mailing list
> yt-dev at lists.spacepope.org
> http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.spacepope.org/pipermail/yt-dev-spacepope.org/attachments/20160710/f6d3be5b/attachment.html>

More information about the yt-dev mailing list