Hi, I am planning on contributing some changes to measure.regionprops, mainly for reducing memory usage. I have noticed a couple of really odd things with respect to coding style in this code. I am tempted to just refactor this to a more conventional style in the course of making my changes, but it looks like someone went to quite some trouble to do it this way, so I thought I would ask here first in case there is some reason for doing it like this. Firstly, the way the methods are made into property methods (i.e. what would normally be simply declared with the @property decorator) is strange. This is currently done by running through the class dir and adding the properties; more strangely the code to do this is burred in a method which also programmatically re-factors the doc strings. This method is set to run only when __debug__ is defined, which means that if python happens to be run with the -O flag, then any code which uses regionprops in the conventional manner (including all unit tests) will fail in a very confusing way. The doc string thing is also strange. All the methods in the _RegionProperties class don't actually have docstrings. The actual attribute documentation is instead in some notes inside the regionprops function. The docstring for this is programmatically pasted into the appropriate __doc__ attributes on module import with a complicated regex. -- Richard
Hi Richard, On April 8, 2019 18:58:37 richard@rdg.cc wrote:
Firstly, the way the methods are made into property methods (i.e. what would normally be simply declared with the @property decorator) is strange. This is currently done by running through the class dir and adding the properties; more strangely the code to do this is burred in a method which also programmatically re-factors the doc strings. This method is set to run only when __debug__ is defined, which means that if python happens to be run with the -O flag, then any code which uses regionprops in the conventional manner (including all unit tests) will fail in a very confusing way.
I think you are right that the property setters should execute in debug mode as well. The function that handles that currently conflates docstrings and property conversion. The principle here is that a piece of information (such as a docstring) should exist in only one place. But other than that, I think there is a lot of room for improvement and to clarify the code---so your contribution will be most welcome. Best regards, Stéfan
Hi Richard, while I can't help with the question whether there's a good reason for the current style or not, I want to bring your attention to the following thread: https://mail.python.org/pipermail/scikit-image/2017-July/005313.html where I reported major memory blow-up when making computations on binary images. Maybe this is something you will come across when contributing. Best regards and the best of luck to you for this endeavor. Martin On 4/9/19 3:57 AM, richard@rdg.cc wrote:
Hi,
I am planning on contributing some changes to measure.regionprops, mainly for reducing memory usage. I have noticed a couple of really odd things with respect to coding style in this code. I am tempted to just refactor this to a more conventional style in the course of making my changes, but it looks like someone went to quite some trouble to do it this way, so I thought I would ask here first in case there is some reason for doing it like this.
Firstly, the way the methods are made into property methods (i.e. what would normally be simply declared with the @property decorator) is strange. This is currently done by running through the class dir and adding the properties; more strangely the code to do this is burred in a method which also programmatically re-factors the doc strings. This method is set to run only when __debug__ is defined, which means that if python happens to be run with the -O flag, then any code which uses regionprops in the conventional manner (including all unit tests) will fail in a very confusing way.
The doc string thing is also strange. All the methods in the _RegionProperties class don't actually have docstrings. The actual attribute documentation is instead in some notes inside the regionprops function. The docstring for this is programmatically pasted into the appropriate __doc__ attributes on module import with a complicated regex.
-- Richard _______________________________________________ scikit-image mailing list -- scikit-image@python.org To unsubscribe send an email to scikit-image-leave@python.org
On Tue, Apr 9, 2019, at 7:35 PM, Martin Fleck wrote:
while I can't help with the question whether there's a good reason for the current style or not, I want to bring your attention to the following thread: https://mail.python.org/pipermail/scikit-image/2017-July/005313.html where I reported major memory blow-up when making computations on binary images.
Yikes, it looks like we totally dropped the ball on that issue back in the day, Martin, sorry about that! I hope you managed to get your work done! To be honest the dtype probably explains most of it and we only just recently got the machinery to support different dtypes in Cython, see https://github.com/scikit-image/scikit-image/pull/3486 It would have been hard to help you before that! Richard,
I am planning on contributing some changes to measure.regionprops, mainly for reducing memory usage. I have noticed a couple of really odd things with respect to coding style in this code. I am tempted to just refactor this to a more conventional style in the course of making my changes, but it looks like someone went to quite some trouble to do it this way, so I thought I would ask here first in case there is some reason for doing it like this.
Thanks for checking! There is a mix of necessary and unnecessary styles in regionprops, so checking is the right approach. The number one thing I want to stress is that the cached_property architecture is very useful because many properties depend on other, expensive-to-compute properties, so it is important that these are cached. For example, many depend on image moments, and removing caching here would be disastrous for performance.
Firstly, the way the methods are made into property methods (i.e. what would normally be simply declared with the @property decorator) is strange. This is currently done by running through the class dir and adding the properties; more strangely the code to do this is burred in a method which also programmatically re-factors the doc strings. This method is set to run only when __debug__ is defined, which means that if python happens to be run with the -O flag, then any code which uses regionprops in the conventional manner (including all unit tests) will fail in a very confusing way.
This is a very good point. If you can keep the caching working and simple while fixing this issue, we would be very grateful!
The doc string thing is also strange. All the methods in the _RegionProperties class don't actually have docstrings. The actual attribute documentation is instead in some notes inside the regionprops function. The docstring for this is programmatically pasted into the appropriate __doc__ attributes on module import with a complicated regex.
Yes, I agree 100% with this. Again, if you can keep a single location for each docstring that renders nicely with our docs, that would be fantastic! Personally I would very much like the docstrings to be with their corresponding property. Thanks, Juan.
participants (4)
-
Juan Nunez-Iglesias -
Martin Fleck -
richard＠rdg.cc -
Stefan van der Walt