-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Img proc math #62
base: master
Are you sure you want to change the base?
Img proc math #62
Conversation
After review of the arithmetic tools with Eric, we decided that tools whos sole purpose is to wrap existing tools into more user friendly VisTrails modules should be located in VTTools, thereby leaving skxray to exclusively contain new analysis tools and functions, while continuing to increase the usefullness and usability of VisTrails for analysis of NSLS-II data. The file mathops.py has been moved to vttools/to_wrap and renamed as image_proc.py. The file test_img_proc.py has been copied from skxray/tests/ to vttools/tests/.
…ails As per review and comment from Eric, the arithmetic functions designed explicitly to ease image processing and data analysis in VisTrails have been moved to the VTTools repo. This commit removes the addl analysis funcs, which will remain in skxray (logical_nand, logical_nor, logical_sub). The remaining functions (arithmetic_basic, arithmetic_custom, and logic_basic) remain. All additional math functions are directly imported from either mathops or numpy. And names for mathops functions have been corrected to mimic style established in Numpy.
Simple term replacement. Replaced all references to mathops by changing import statement to: import vttools.to_wrap.image_proc as img The tests included focus on the vistrails specific functions: arithmetic_basic, logic_basic, and arithmetic_custom
Previously the docstring for arithmetic_custom included references to additional operators such as +=, -=, *= etc. which are supposed to simplify expressions like a=a+b, which is the equivalent of a+=b. However, the parsing function does not evaluate these short cuts correctly. If these additional operators become important, then I'll revisit and modify the parsing function, or write a custom one. However, at this time the additional operators are deemed to be unnecessary, since the expressions can still be evaluated without the operators.`
As per Eric's suggestion, inputs specific to data have been renamed in order to conform to typical numpy docstrings, e.g. the arg src_data1 and src_data2 have been changed to x1 and x2. The funcs requiring a string operation to be defined have also been standardized so that the input order starts with the operation or expression followed by the input data. Finally, the docstring for arithmetic_custom was incomplete, and has been flushed out.
As discussed with Eric, we decided to shorten, simplify and make the function names more explicit about their functionality. So, logic_basic has been renamed to logic, arithmetic_basic has been renamed arithmetic, and arithmetic_custom has been changed to arithmetic_expression. In Vistrails these tools are exprected to be nested in the tool tree under an "Image Arithmetic" heading, included in the "Image Processing" tool set/kit.
After review of the arithmetic tools with Eric, we decided that tools whos sole purpose is to wrap existing tools into more user friendly VisTrails modules should be located in VTTools, thereby leaving skxray to exclusively contain new analysis tools and functions, while continuing to increase the usefullness and usability of VisTrails for analysis of NSLS-II data. The file mathops.py has been moved to vttools/to_wrap and renamed as image_proc.py. The file test_img_proc.py has been copied from skxray/tests/ to vttools/tests/.
…ails As per review and comment from Eric, the arithmetic functions designed explicitly to ease image processing and data analysis in VisTrails have been moved to the VTTools repo. This commit removes the addl analysis funcs, which will remain in skxray (logical_nand, logical_nor, logical_sub). The remaining functions (arithmetic_basic, arithmetic_custom, and logic_basic) remain. All additional math functions are directly imported from either mathops or numpy. And names for mathops functions have been corrected to mimic style established in Numpy.
Simple term replacement. Replaced all references to mathops by changing import statement to: import vttools.to_wrap.image_proc as img The tests included focus on the vistrails specific functions: arithmetic_basic, logic_basic, and arithmetic_custom
Previously the docstring for arithmetic_custom included references to additional operators such as +=, -=, *= etc. which are supposed to simplify expressions like a=a+b, which is the equivalent of a+=b. However, the parsing function does not evaluate these short cuts correctly. If these additional operators become important, then I'll revisit and modify the parsing function, or write a custom one. However, at this time the additional operators are deemed to be unnecessary, since the expressions can still be evaluated without the operators.`
As per Eric's suggestion, inputs specific to data have been renamed in order to conform to typical numpy docstrings, e.g. the arg src_data1 and src_data2 have been changed to x1 and x2. The funcs requiring a string operation to be defined have also been standardized so that the input order starts with the operation or expression followed by the input data. Finally, the docstring for arithmetic_custom was incomplete, and has been flushed out.
As discussed with Eric, we decided to shorten, simplify and make the function names more explicit about their functionality. So, logic_basic has been renamed to logic, arithmetic_basic has been renamed arithmetic, and arithmetic_custom has been changed to arithmetic_expression. In Vistrails these tools are exprected to be nested in the tool tree under an "Image Arithmetic" heading, included in the "Image Processing" tool set/kit.
- This looks like a lot of changes, but there is functionally no difference between the original code and this code. It is entirely stylistic. - Doc edits moved the descriptive prose to `/doc/resource/user-guide/image.rst` and used language largely copied from numpy - content edits favored use of built in functionality over defining look-up dictionaries
Conflicts: vttools/tests/__init__.py
Conflicts: vttools/tests/test_img_proc.py vttools/to_wrap/image_proc.py
The tools designed to ease use of math functions in VisTrails are now included in vttools-->to_wrap-->image_processing-->arithmetic since these tools are explicitly designed to simplify the VisTrails user experience. As part of the growing collection of image processing functions included in skxray, the basic image arithmetic functions are compiled in skxray-->image_processing-->arithmetic for completeness.
You seem to have added an html/ folder to this branch in bdbe90d. Can you remove that? |
See Also | ||
-------- | ||
- User guide section on "Image Operations" | ||
('/doc/resource/user-guide/image.rst') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a todo here noting that this needs to be updated when there are sphinx docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I thought you added that. Which is why I left it in....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs updated with the TODO statement.
HTML folder was accidentally added during a previous commit. This folder has now been removed in order to merge this PR finally.
As Eric mentioned, lmfit.asteval may be a cleaner and more effective parsing tool for evaluating the input expressions. Added an explicit TODO noting the parser comparision should be completed on next iteration.
I'm assuming that Travis is failing because the required pkg hasn't been merged into skxray? Just want to make sure. |
Supersedes #57 and #54