<div dir="ltr">Hi Cameron,<div><br></div><div>Thanks for doing this; you are not the "bad guy." I really appreciate that you took the time out to itemize these things. Each of these is important and necessary to do, and I like having them all be bitbucket issues.</div><div><br></div><div>From what I can tell, *nearly* all of these are *not* API breaking, is that right? If so, can we decide on what are blockers to the merging of the vr refactor into the yt head, and what are blockers on release? If it's "everything" as blocker to merging into the yt head, then OK, but I think there may be some leeway. The main reason I am suggesting we do this is because *with* the backport script now running regularly, we are in a very, very good position to move everyone running yt for production onto "stable" and then encourage active, possibly-flaky, development on the "yt" branch.</div><div><br></div><div>What do you think?</div><div><br></div><div>-Matt</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 2, 2015 at 4:40 PM, Cameron Hummels <span dir="ltr"><<a href="mailto:chummels@gmail.com" target="_blank">chummels@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hey everyone,<div><br></div><div>Short Version of this email: </div><div><br></div><div>Since it seems like I'm the minority in trying to flush out some of the remaining problems with the VR infrastructure before it gets merged into the main head of the dev branch, I will not be an impediment anymore. I've provided a list of things I think should be addressed (or discussed at least) at the bottom of this email before merging, but as I said, I will no longer block the merge if people want to do it.</div><div><br></div><div>Long Version:</div><div><br></div><div><div>A few years ago a few of us decided that the volume rendering infrastructure was too clunky and difficult to use efficiently to make good volume renders, so we detailed how it should be improved in a YTEP (<a href="http://ytep.readthedocs.org/en/latest/YTEPs/YTEP-0010.html" target="_blank">http://ytep.readthedocs.org/en/latest/YTEPs/YTEP-0010.html</a>). Subsequently, Sam Skillman put in a huge amount of work (with help from Matt, me, Andrew, Suoqing, and more) overhauling the volume rendering infrastructure to match the description of the YTEP. The new VR interface appears to work well in many cases, but I believe there are still a lot of remaining issues to be addressed before it's ready for mainstream: bugfixes, docs, recipes, etc. </div><div><br></div><div>About 6 months ago, there was a pull request (<a href="https://bitbucket.org/yt_analysis/yt/pull-requests/1256/volume-rendering-refactor/diff" target="_blank">https://bitbucket.org/yt_analysis/yt/pull-requests/1256/volume-rendering-refactor/diff</a>) made to merge all of the new volume rendering infrastructure into the development branch, but at that point, the PR was enormous and difficult to review. While there were still a lot of unresolved issues associated with the PR, it was decided to merge it into the "experimental" bookmark on the dev branch, so as to not hold up other development dependent on that infrastructure, and to make it easier to test it out for everyone. </div><div><br></div><div>While I was reviewing this pull request, I tried my best to read through all of the code diffs, make sure there was appropriate documentation (docstrings, narrative, comments, etc) for such a significant change, and actually download and test run the example code to see if it did what it was supposed to do. This was a ton of work, but I think that this level of review needs to occur with major PRs to assure things are working before merging and exposing problematic code to the user base. If we put code out there and don't properly explain how to use it (or if it doesn't work), we're ultimately going to make a lot more work for ourselves trying to respond to people's questions on the mailing list about how to use it. And we'll potentially sour a lot of people to the code when they try to use something but find it doesn't do what they want it to do. </div><div><br></div><div>Now there is increased pressure to just merge the VR refactor into the main dev branch so as to streamline overall development, and it seems I am the only one who has been resistant to this. The argument for the merge has been: "once it is in the main branch, people will sort out the bugs." Once something is in the main branch, it may very well get more people trying to use it; however, the level of confusion/irritation will increase if there is insufficient documentation, and the ability to change any kind of API is severely limited because it breaks peoples scripts to do so. The time for reviewing and fixing code comes before it gets merged into the main dev branch.</div><div><br></div><div>But ultimately, I do not want to be seen as the bad guy anymore on this. If people want to merge the code, then I don't want to hold it up. I made a list of items when I reviewed the PR a few months back, and below are the things I think are remaining to be done. But what I think really needs to be done is to have other people do a full code review (including testing) and find things themselves (maybe my list, maybe other things). And then everyone works to fix them. If people want to address my list, or they want to ignore them, that's fine. Really, it doesn't matter to me anymore.</div><div><br></div><div>Cameron</div><div><br></div><div><br></div><div>List of remaining things I think should be addressed before merging experimental head with main head on dev branch:<br><ul style="margin:10px 0px 0px"><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">The cookbook recipe for a simple_vr should get rid of all of the extraneous stuff and just have the most simple means of creating a VR, just like the simple slice and simple proj do.</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">clip_ratio</code> could be changed to something more descriptive (stddev_mask?), and should be defaulted to something like 4.0 since it is used almost everywhere to produce useful images</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:14.6666666666667px;font-family:Arial;color:rgb(0,0,0);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">When making a VR, it defaults to using the first field in the field_name list if a field is not specified, which almost never gives the desired result. Perhaps just default to ('gas', 'density') like the PW plots do? Implicitly assumed in examples (ie cookbook recipe: camera_movement.py)</span></p></span></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">map_to_colormap</code><span style="line-height:20.4400005340576px;background-color:transparent"> function is not in the API docs.</span></p></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">map_to_colormap</code> has a <code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">scale</code> keyword, which could be more descriptive or have examples. what does it do? <span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:1.38;background-color:transparent">30 gives good results, but 10 doesn't and the default: 1. renders a black screen. how does the user figure out what to use? even ballpark of what to use? change the defaults to be useful?</span></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="line-height:20.4400005340576px">There should be a default filename for saving images. Default filename should be something descriptive, like</span><span style="line-height:20.4400005340576px"> </span><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">PlotWindow</code><span style="line-height:20.4400005340576px"> </span><span style="line-height:20.4400005340576px">filenames default to</span><br></p></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="line-height:20.4400005340576px">Docstrings for all outward facing functions and classes including a description, parameters, returned quantities, and an example in the docstrings. right now, </span><span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:20.4400005340576px;background-color:transparent">many examples are so trivial so as to not be helpful at all: "source = PointsSource(particle_positions)" examples would be better served to be a full VR with the appropriate method/class injected; probably 3-5lines each; then can be tested with docstring tests down the road too.</span></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:20.4400005340576px;background-color:transparent"><span><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:14.6666666666667px;vertical-align:baseline;background-color:transparent">off_axis_projection() has a resolution arg, perhaps make a resolution kwarg with a default set to 512? 256? just as PW has and VR has?</span></p><span style="font-size:14.6666666666667px;vertical-align:baseline;background-color:transparent"></span></span></span></li><li style="word-wrap:break-word"><span style="font-size:14.6666666666667px;line-height:20.4400005340576px;background-color:transparent">off_axis_projection has an "item" kwarg, which should really be "fields" and should be handled in the same way as PW does (recognizes lists of fields)</span><br></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="line-height:20.4400005340576px">The</span><span style="line-height:20.4400005340576px"> </span><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">__init__</code><span style="line-height:20.4400005340576px"> </span><span style="line-height:20.4400005340576px">method on many VR classes have the docstrings, but it would be better to have them just after the class declaration so that the API docs rendered them properly.</span></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="line-height:20.4400005340576px"><span><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:14.6666666666667px;font-family:Arial;color:rgb(0,0,0);vertical-align:baseline;white-space:pre-wrap;background-color:transparent">the way to render an off_axis_projection requires yt.write_image() instead of sc.render() like the VRs even though it is using the same infrastructure. inconsistent. why? sc.render() *runs*, but even with a clip_ratio, it just gives a black screen because all of the values are NaNs</span></p></span></span></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="line-height:20.4400005340576px">Transfer functions operate in log space for fields that are logged. This should be done in the linear space of the fields. Can be very confusing.</span><br></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">TransferFunctionHelper</code> should be split out into a different page in the narrative docs, as the jupyter notebook in the middle of the docs breaks it up and makes it confusing</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Put the VR annotations (annotate_grids()) in the narrative docs to show how to do them.</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">rotate</code> on the camera should specify what it is rotating -- specifically, it rotates the "position" of the camera, not the direction of the camera</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">switch_view</code> should be included in narrative docs</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Add examples for all the different lenses to the cookbook or narrative docs</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Make one parallelization header, with subheaders in the narrative docs</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Note differences in ways to write images in VR: (1) <span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;line-height:1.38;white-space:pre-wrap;background-color:transparent">you specify an fname in the yt.volume_render() function originally (2) </span><span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:1.38;background-color:transparent">you get the resulting im object from yt.volume_render() and then write it using im.write_png(fname), (3) </span><span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:1.38;background-color:transparent">you take the resulting sc object from yt.volume_render() and then sc.render(fname). </span><span style="color:rgb(0,0,0);font-family:Arial;font-size:14.6666666666667px;white-space:pre-wrap;line-height:20.4400005340576px;background-color:transparent">i guess it is good to have these three methods for getting the images to file, but it might be worthwhile to explicitly state that there are three methods for writing your VR to file, so people don't get confused on which is the main method (as i did and just had to figure it out by looking through different examples, source, etc).</span></li></ul><ul style="margin:10px 0px 0px"><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Camera movement cookbook needs to be improved to make images that make sense</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Link to subsections of the VR docs in the relevant cookbook recipes not just to VR as a whole.</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><span style="line-height:20.4400005340576px">Add narrative docs to get the source, camera, get transfer function, etc.</span><br></li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Possibly get rid of all getters other than for <code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">source</code> on the scene object.</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word"><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">get_source</code> is incorrect, since dicts are not sorted</li><li style="color:rgb(51,51,51);font-family:Arial,sans-serif;font-size:14px;line-height:20.4400005340576px;word-wrap:break-word">Left handed versus right handed images; There was significant discussion regarding this in the VR refactor PR linked above. The conclusion that a few of us reached (Suoqing, Michael, myself) was: <span style="line-height:20.4400005340576px"> The VR system measures the normal vector from the camera, but in the PlotWindow interface, there is no "camera" per se, and the normal vector is measured from the </span><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">focus</code><span style="line-height:20.4400005340576px">. So they're both in a RHS from their own perspective, but it's because that normal vector is getting reversed between the two systems. The</span><span style="line-height:20.4400005340576px"> fix would be to flip the direction of </span><code style="font-family:Consolas,Menlo,'Liberation Mono',Courier,monospace;font-size:9pt;line-height:1.4;border:1px solid rgb(204,204,204);border-radius:3px;padding:1px 3px;background:rgb(245,245,245)">north_vector</code><span style="font-family:arial,sans-serif;font-size:small;line-height:normal;color:rgb(34,34,34)"> </span><span style="font-family:arial,sans-serif;font-size:small;line-height:normal;color:rgb(34,34,34)">in the VR system so it's right handed from the perspective of the focus, but left handed from the perspective of the camera. Then we get the same results from similar inputs to PlotWindow outputs and VR (including off_axis_projections()).</span></li><li style="word-wrap:break-word">Using opaque sources (e.g. points and lines and grids) with render sources (e.g. gas VR) tends to result in either seeing just one or the other out of the box. It takes a lot of work to balance the opacity of the different sources, and it is not documented how to do this. I think we should potentially provide better default opacities associated with these so as to make both points and gas visible in the same rendering out of the box. I think we should also work on documenting easy ways to change the relative opacity of different sources to highlight one source or the other in the rendering.</li></ul></div><span><font color="#888888"><div><br></div>-- <br><div><div dir="ltr"><div>Cameron Hummels<div><span style="font-size:12.8000001907349px">NSF Postdoctoral Fellow</span></div><div><span style="font-size:12.8000001907349px">Department of Astronomy</span></div><div><span style="font-size:12.8000001907349px">California Institute of Technology</span><br></div><div><a href="http://chummels.org" style="font-size:12.8000001907349px" target="_blank">http://chummels.org</a><br></div></div></div></div>
</font></span></div></div>
<br>_______________________________________________<br>
yt-dev mailing list<br>
<a href="mailto:yt-dev@lists.spacepope.org" target="_blank">yt-dev@lists.spacepope.org</a><br>
<a href="http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org" rel="noreferrer" target="_blank">http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org</a><br>
<br></blockquote></div><br></div></div>