<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 26, 2017 at 4:55 AM, Jaime Fernández del Río <span dir="ltr"><<a href="mailto:jaime.frio@gmail.com" target="_blank">jaime.frio@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">Hi all,<div><br></div><div>I have started sending PRs for ndimage. It is my intention to keep at it through the summer, and would like to provide some context on what I am trying to achieve.</div><div><br></div><div>A couple of years ago I mentored a GSoC to port ndimage to Cython that didn't do much progress. If nothing else, I think I did learn quite a bit about ndimage and what makes it hard to maintain. And I think one of the big ones is lack of encapsulation in the C code: ndimage defines four "classes" in <a href="https://github.com/scipy/scipy/blob/master/scipy/ndimage/src/ni_support.h" target="_blank">ni_support.h</a> that get used throughout, namely NI_Iterator, NI_LineBuffer, <wbr>NI_FilterIterator and NI_CoordinateList.</div><div><br></div><div>Unfortunately they are not very self contained, and e.g. to instantiate a NI_FilterIterator you typically have to:</div><div><ul><li>declare a NI_FilterIterator variable,</li><li>declare two NI_Iterator variables, one for the input array, another for the output,</li><li>declare a npy_intp pointer of offsets and assign NULL to it,</li><li>call NI_InitFilterOffsets to initialize the offsets pointer,<br></li><li>call NI_InitFilterIterator to initialize the filter iterator,</li><li>call NI_InitPointIterator twice, once for the input, another for the output,</li><li>after each iteration call NI_FILTER_NEXT2 to advance all three iterators,</li><li>after iteration is finished, call free on the pointer of offsets.</li></ul><div>There is no good reason why we couldn't refactor this to being more like:</div></div><div><ul><li>call NI_FilterIterator_New and assign its return to a NI_FilterIterator pointer,</li><li>after each iteration call NI_FilterIterator_Next to advance all involved pointers,</li><li>after iteration is finished, call NI_FilterIterator_Delete to release any memory.</li></ul><div>Proper encapsulation would have many benefits:</div></div><div><ul><li>high level functions would not be cluttered with boilerplate, and would be easier to understand and follow,</li><li>chances for memory leaks and the like would be minimized,</li><li>we could wrap those "classes" in Python and test them thoroughly,</li><li>it would make the transition to Cython for the higher level functions, much simpler.</li></ul><div>As an example, the lowest hanging fruit in this would be <a href="https://github.com/scipy/scipy/pull/7527" target="_blank">#7527</a>.</div><div><br></div><div>So open source wise this is what I would like to spend my summer on. </div></div></div></blockquote><div><br></div><div>Awesome! ndimage definitely could use it!<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Any feedback is welcome, but I would especially like to hear about:</div></div><div><ul><li>thoughts on the overall plan,</li></ul></div></div></blockquote><div><br></div><div>Sounds like a great plan. I do think that the current test suite is a bit marginal for this exercise and review may not catch subtle issues, so it would be useful to:<br></div><div>- use scikit-image as an extra testsuite regularly<br></div><div>- ideally find a few heavy users to test the master branch once in a while<br></div><div>- use tools like a static code analyzer and Valgrind where it makes sense.<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><ul><li>reviewer availability: I would like to make this as incremental as possible, but that means many smaller interdependent PRs, which require reviewer availability,</li></ul></div></div></blockquote><div>For the next 4 weeks or so my availability will be pretty good. I'm pretty sure that I don't know as much about ndimage as you do, but that's likely true for all other current devs as well:) I think it's important to keep up with your PRs; once we start getting too far behind in reviewing the effort only goes up. I suggest not being too modest in pinging for reviews of PRs that are going to be a bottleneck or result in conflicts later on.<br><br></div><div>Ralf<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><ul><li>if anyone wants to join in the fun, I'm more than happy to mentor!</li></ul><span class="HOEnZb"><font color="#888888"><div>Jaime</div></font></span></div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- <br><div class="m_-7599524321668460780gmail-m_2096557553935176421gmail_signature">(\__/)<br>( O.o)<br>( > <) Este es Conejo. Copia a Conejo en tu firma y ayúdale en sus planes de dominación mundial.</div>
</div></font></span></div>
<br>______________________________<wbr>_________________<br>
SciPy-Dev mailing list<br>
<a href="mailto:SciPy-Dev@python.org">SciPy-Dev@python.org</a><br>
<a href="https://mail.python.org/mailman/listinfo/scipy-dev" rel="noreferrer" target="_blank">https://mail.python.org/<wbr>mailman/listinfo/scipy-dev</a><br>
<br></blockquote></div><br></div></div>