Remove interactive volume rendering to repackage it elsewhere

[matthewturk]

This is a big PR, and potentially unreviewable except by looking just at the new files. I've tagged this with "proposal" for some reasons that will be obvious below. But, it works (for more people than just me!) and I think it's pretty promising.

There are likely bits of dead code that can be removed, and maybe things that need to be fixed, and I'm also thinking that I will add an "extras" installation bit to setup.py to make it easier to run the VR with an installation. Also, the docs: lacking!

So here's what I'm trying to figure out: should this be in yt, or should it be split out? As it stands, it only works with yt, but also only uses public-ish APIs. We have an interactive volume rendering system already, and this fixes it in a number of ways, but should we?

If we say it should go in, I'll consider this a draft PR -- and I'll make sure that we have:

• docs and examples
• fixed up dependencies
• removed dead code

But if it goes into its own thing, then I'll work on splitting it out. It's a bit tricky to retain the history, so maybe a no-op merge, or tacking onto this a removal of the existing interactive VR would be a nice balance, but, whatever.

I'll follow up with some info on how to run this for folks who might want to try it out.

[matthewturk]

I've pushed from your branch to this one, @cphyc . Did you already address the comments you posted above?

[cphyc]

I've pushed from your branch to this one, @cphyc . Did you already address the comments you posted above?

I did not!

[matthewturk]

Cool, I'll do those next.

…

On Thu, Sep 17, 2020 at 3:01 AM Corentin Cadiou ***@***.***> wrote: I've pushed from your branch to this one, @cphyc <https://github.com/cphyc> . Did you already address the comments you posted above? I did not! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2896 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO7EQ7M33MZBWEVJIVLSGG65TANCNFSM4RIJICLA> .

[munkm]

Ok, so this is a great discussion starter already! Thanks matt!!

I see a few points that are worth making here:

• If we do want to interface with other APIs (potentially having users use the volume renderer with other packages), then having a light, standalone package makes sense. If the ultimate goal is just to have a volume renderer for yt purposes then i think there are benefits to leaving it in or splitting it out.
• We have discussed at length about making yt more modular. I think splitting out the volume renderer would be in line with that philosophy. We could still include docs in the primary yt repo using the volume renderer so users are exposed to the package. This could be our first step towards a larger "yt ecosystem" of packages.
• There will be faster development iteration on the volume renderer because you won't have the same review requirements for merging as you do in the yt main repository.

I am personally in favor of moving the volume renderer into its own package. I hear the concerns about losing the history, and I do find that challenging. However, if we do want to take the path of a standalone package farther down the road then splitting it out sooner will preserve more of its history long term.

[matthewturk]

Thanks for the thoughts everyone! @cphyc has asked a couple questions that I'm going to respond to, and then I'll respond to @munkm .

1. What is our current state?
2. What does this PR bring us to?

So the current state of the IDV in yt is that it's essentially unmaintained, and it also has some mathematical errors in the actual raycasting that make it look bad. It's also not really able to do compositing, and adding new shaders is pretty painful.

Perhaps most importantly, it uses CyGLFW3, which is not extremely widely available -- it is pretty nice! -- and it has some code that is fairly specific to that, although not much.

One very specific thing that is different about the "old" method is that the mechanism by which we notified items of changes -- in colormap, in view, etc -- is manually implemented.

I started working on the actual raycasting algorithm (the fix was not that bad!), but in the service of that I ended up needing more and more functionality that I just started implementing, and rewriting bits of it to support that. The biggest set of changes:

• Everything has been "traitlets"-ized, so it is threaded through with lots of data binding that cause auto-updates and whatnot
• I've added lots and lots of OpenGL helpers and whatnot, so that shaders can have introspected parameters and textures bound back and forth and whatnot
• It now uses pyglet (actually version 2 of pyglet, which is not quite yet released) for managing windows and input/output
• It has a simple GUI using pyimgui, which maps a couple of the traitlets things onto an actual GUI that you can modify
• There are now some additional annotations available
• Lots of little things, like data object selection now passes a bitmap in to the shaders so you can do actual selection, and this also opens up some cool possibilities for inline data selection
• You can render off-screen (or you could, maybe I should double-check that it's all still functional...) and pass it back as a buffer to, say, a jupyter notebook. I have not implemented a lot with this, but it did allow OpenGL-speed rendering on remote data.

One of the downsides to some of the traitlets-izing stuff is that some operations like moving the camera can introduce latency that was not there; I think this is a concern for another time, though.

@munkm brings up three points:

• If we do want to interface with other APIs (potentially having users use the volume renderer with other packages), then having a light, standalone package makes sense. If the ultimate goal is just to have a volume renderer for yt purposes then i think there are benefits to leaving it in or splitting it out.
• We have discussed at length about making yt more modular. I think splitting out the volume renderer would be in line with that philosophy. We could still include docs in the primary yt repo using the volume renderer so users are exposed to the package. This could be our first step towards a larger "yt ecosystem" of packages.
• There will be faster development iteration on the volume renderer because you won't have the same review requirements for merging as you do in the yt main repository.

The first one, I think, is aspirational -- but a possibility. We use some functionality that's available only in yt, like kD-tree traversal, but we don't need to.

The second and third ones speak to me a lot more. I think they are the biggest argument for splitting it out.

What you bring up about the history is something I've worried a bit about, since there is a fair bit in here that I don't really want to lose.

What if we were to merge, then remove the entire interactive volume rendering subsystem (not the software volume rendering subsystem), and start afresh? Then the history would still exist in the main yt repository, but it would exist separately and be developed separately. Would that be a satisfactory option?

[munkm]

What if we were to merge, then remove the entire interactive volume rendering subsystem (not the software volume rendering subsystem), and start afresh? Then the history would still exist in the main yt repository, but it would exist separately and be developed separately. Would that be a satisfactory option?

This is a good option. You mentioned in the opening of this PR that this is potentially unreviewable. How do you recommend reviewers proceed with the review of this PR for it to be merged?

[matthewturk]

If we do go with that option, I think we could potentially just say, let's do it, and I'll split this out and update this PR to just be a removal of the whole subsystem.

Or, if folks want to review, I think it would be awesome to get some comments on the structure of the code, then I'll update to include some more docs and requirements, and then we can do more specific reviewing.

I'm fine with either! The docs and requirements will have to happen either way! But if we opt for that option, we can move forward right away.

[neutrinoceros]

For what it's worth I think this would make a very nice package of its own. As previously mentioned, having it separate from the main dir should allow for quicker iterations. Also, I'm game to review this down the road !

[brittonsmith]

In case it's not too late to chime on in the issue of where this should go, I'm in favor of splitting it off its own package. Personally, I would be fine if this is first accepted and then removed if it helps facilitate removal of the old machinery.

[matthewturk]

OK, I'll update the PR and split it all out.

[matthewturk]

OK! I have started splitting it off into https://github.com/data-exp-lab/yt_idv . Once that is ready, I'll update this PR, and we'll be good to go.

[munkm]

We should also update the extensions page on the website to include the interactive volume renderer, once it is split out.

[matthewturk]

So this now removes all of the interactive VR from the source, and it's currently in https://github.com/data-exp-lab/yt_idv/ . There are a couple places that I need to remove a few more things in the docs and cookbook, but I think they should wait until I get a 0.1.0 of yt_idv out so that I can update them rather than remove them.

[munkm]

I need to remove a few more things in the docs and cookbook, but I think they should wait until I get a 0.1.0 of yt_idv out so that I can update them rather than remove them.

For this do we want to include specific directions (or a link to the yt_idv docs) on installing the volume rendering extension too? Do you want to merge this before the docs updates?

[munkm]

👀 👀 👀

[matthewturk]

If possible, I would like to merge before the docs update. But, I'm still a little bit from getting yt_idv properly released ... I was hoping by the end of this week.

[matthewturk]

yt_idv is now released in an initial form! I'm also making additional updates to it to move it along. So, I think I'm feeling OK about this going in.

[neutrinoceros]

Out of curiosity, is there a reason why yt_idv is not hosted under the yt-project umbrella yet ?

[munkm]

is there a reason why yt_idv is not hosted under the yt-project umbrella yet ?

This is a good point! I think it would be worth moving it into the org.

[matthewturk]

I think it should be moved there too! I will do so ASAP.

…

On Wed, Dec 16, 2020 at 10:52 AM Madicken Munk ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2896 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVXO5K444PDCXSF33DVT3SVDQVPANCNFSM4RIJICLA> .