Skip to content

Conversation

@archit120
Copy link
Contributor

@archit120 archit120 commented May 28, 2020

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

  • I agree to contribute to the project under OpenCV (BSD) License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@vpisarev
Copy link
Contributor

@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.

Copy link

@americast americast left a 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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$ export JULIA_LOAD_PATH=$PWD\OpenCV
$ export JULIA_LOAD_PATH=$PWD/OpenCV

Copy link
Contributor Author

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.

Copy link
Contributor

@vpisarev vpisarev Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@archit120, by the way:

  1. Note that this works only if you called make install
    is it really true? I remember, I made it working without running make install

  2. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You called Pkg.develop that's functionally equivalent to make install

  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@archit120
Copy link
Contributor Author

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.

Copy link
Member

@alalek alalek left a 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")
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

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_.

@archit120
Copy link
Contributor Author

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?

@archit120
Copy link
Contributor Author

@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?

@vpisarev
Copy link
Contributor

👍

@vpisarev vpisarev self-requested a review June 13, 2020 19:31
@vpisarev
Copy link
Contributor

@alalek, looks like all the requests have been addressed. Can you please take a look at it?

@archit120
Copy link
Contributor Author

@vpisarev there's one comment left. I plan to get it done by tomorrow.

endif()

if(NOT JlCxx_DIR)
execute_process(
Copy link
Member

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.

set(the_description "The Julia bindings")
ocv_add_module(julia
opencv_core
opencv_imgproc
Copy link
Member

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.

@archit120
Copy link
Contributor Author

@alalek I made some indentation changes. Please let me know if this is fine or you want something else.

@alalek alalek merged commit 522a062 into opencv:master Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants