| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Julia Bindings GSoC - phase1 #2547
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
Conversation
a few fixes to compile julia bindings on mac
|
@alalek, this is the first drop of Julia bindings for OpenCV, just proof of concept to check build scripts etc. Only a few functions are wrapped so far, and they are wrapped manually. I checked it on Linux an Mac – work well, and the tests pass. Windows support would probably be difficult to add because of some limitations in the depedencies: libcxxwrap-julia and CxxWrap. |
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.
Hey @archit120, thanks a lot for the PR! It looks really awesome! ❤️
It would be great if you could mention somewhere the features this module currently supports. Some documentation with simple examples would be helpful. I understand some use cases have been provided in the test, but it would be nice to know the currently supported functions and their limitations, as per your knowledge.
modules/julia/README.md
Outdated
|
|
||
| Note that this works only if you called `make install`. To run the wrapper package without making the installation target you must first set the environment variable `JULIA_LOAD_PATH` to the directory containing the OpenCV package. For example if in the build directory | ||
| ```bash | ||
| $ export JULIA_LOAD_PATH=$PWD\OpenCV |
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.
| $ export JULIA_LOAD_PATH=$PWD\OpenCV | |
| $ export JULIA_LOAD_PATH=$PWD/OpenCV |
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.
Will do this change along with any other required revisions to not clutter commit history.
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.
@archit120, by the way:
-
Note that this works only if you called
make install
is it really true? I remember, I made it working without runningmake install -
export JULIA_LOAD_PATH=$PWD\OpenCV
I think, even more correct would be to put
export JULIA_LOAD_PATH=$PWD:$JULIA_LOAD_PATH,
because OpenCV is the name of the package. It should not be a part of the load path
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.
-
You called
Pkg.developthat's functionally equivalent tomake install -
The load path variable creates a virtual environment like with python. It needs to be pointed to the directory with the Project.toml file so this is correct aswell. In usual circumstances, this variable should not be set at all.
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.
Done
|
Hi @americast The current functionality will probably be obsolete by the end of phase 3 and phase 3 involves documentation. In addition, the samples already use all of the implemented functionality. As such, I do not think that it is necessary to document beyond what is already in the README as well as samples and tests. However, feel free to suggest that again in which case I'll add some documentation in this pull-request. |
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.
Thank you for contribution!
|
|
||
| math(EXPR ARCH "${CMAKE_SIZEOF_VOID_P} * 8") | ||
| if (${ARCH} EQUAL 32 AND ${Julia_WORD_SIZE} MATCHES "64") | ||
| warn_mixed_precision("32" "64") |
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.
warn_mixed_precision
Not defined here.
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.
Done
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.
Please use different name to avoid conflicts. Add prefix like ocv_julia_.
|
The most recent build seems to have failed because of a git error. Is there any way to run it again without creating a new commit? |
|
@vpisarev I have fixed all the empty first lines and return lines that I could find. Do let me know if I missed any file? |
|
👍 |
|
@alalek, looks like all the requests have been addressed. Can you please take a look at it? |
|
@vpisarev there's one comment left. I plan to get it done by tomorrow. |
modules/julia/CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| if(NOT JlCxx_DIR) | ||
| execute_process( |
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.
Please use 2 spaces indentation for commands.
modules/julia/CMakeLists.txt
Outdated
| set(the_description "The Julia bindings") | ||
| ocv_add_module(julia | ||
| opencv_core | ||
| opencv_imgproc |
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.
Please use indentation of 4 spaces for arguments.
|
@alalek I made some indentation changes. Please let me know if this is fine or you want something else. |
This pull request is for the necessary functionality of phase 1 - Julia Bindings GSoC project. The README contains instructions to build and test along with some more explanation of how this project works. Dependencies are CxxWrap.jl and libcxxwrap_julia
Evolution Proposal
Issue
Proposal Abstract
Mentors: @vpisarev @americast
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.