Skip to content

Update calcBackProject_Demo1.py#17055

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
vnikoofard:patch-1
May 14, 2020
Merged

Update calcBackProject_Demo1.py#17055
opencv-pushbot merged 1 commit intoopencv:3.4from
vnikoofard:patch-1

Conversation

@vnikoofard
Copy link
Copy Markdown
Contributor

@vnikoofard vnikoofard commented Apr 14, 2020 •
edited by alalek
Loading

To round a ndarray it's necessary to use np.round() instead to Built-in Python round()

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

@vnikoofard
Update calcBackProject_Demo1.py
575a609 
To round a ndarray it's necessary to use np.round() instead to Built-in Python round()
@asmorkalov asmorkalov self-requested a review April 23, 2020 12:52
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 23, 2020

The issue is reproducible with Python3 only:

Traceback (most recent call last):
  File "./calcBackProject_Demo1.py", line 65, in <module>
    Hist_and_Backproj(bins)
  File "./calcBackProject_Demo1.py", line 34, in Hist_and_Backproj
    cv.rectangle(histImg, (i*bin_w, h), ( (i+1)*bin_w, h - int(round( hist[i]*h/255.0 )) ), (0, 0, 255), cv.FILLED)
TypeError: type numpy.ndarray doesn't define __round__ method

Python2 works as expected.

@asmorkalov asmorkalov added category: python bindings category: samples pr: needs rebase Rebase patch (and squash fixup commits) on the top of target branch labels Apr 23, 2020
asmorkalov
asmorkalov reviewed Apr 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

The issue is reproducible with branch 3.4 and the patch should go into 3.4 branch first. We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no need to re-open PR, apply changes "inplace".

@sergregory
Copy link
Copy Markdown

sergregory commented Apr 23, 2020

@vnikoofard I'd say it can be implemented in a clearer and more performant way:

    bin_heights = np.round(hist * h / 255.0)[:, 0].astype(int)
    for i in range(bins):
        cv.rectangle(histImg, (i*bin_w, h), ( (i+1)*bin_w, h - bin_heights[i] ), (0, 0, 255), cv.FILLED)

What do you think?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 27, 2020

@vnikoofard Do you have a chance to look at proposal from sergregory?

@vnikoofard
Copy link
Copy Markdown
Contributor Author

vnikoofard commented Apr 27, 2020

Sorry for delay. Yes, I agree with @sergregory. It seems more efficient.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 29, 2020

@vnikoofard Could you rebase your branch to 3.4 and update the solution as @sergregory proposed?

@vnikoofard
Copy link
Copy Markdown
Contributor Author

vnikoofard commented Apr 29, 2020

@vnikoofard Could you rebase your branch to 3.4 and update the solution as @sergregory proposed?

I'm a complete newbie! Sorry for my ignorance but on my top of the screen, below the title of commit is written:
vnikoofard wants to merge 1 commit into opencv:3.4 from vnikoofard:patch-1
It doesn't mean it is already in 3.4 base?

@asmorkalov asmorkalov removed the pr: needs rebase Rebase patch (and squash fixup commits) on the top of target branch label Apr 29, 2020
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Apr 29, 2020

Yes, You are right, it's already targeted to 3.4. I removed pr: needs rebase label. What about sergregory's proposal?

@asmorkalov asmorkalov self-requested a review May 14, 2020 11:43
asmorkalov
asmorkalov approved these changes May 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov left a comment

Choose a reason for hiding this comment

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

👍

@opencv-pushbot opencv-pushbot merged commit 65b3209 into opencv:3.4 May 14, 2020
@alalek alalek mentioned this pull request May 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov approved these changes

Assignees

@asmorkalov asmorkalov

Labels

category: python bindings category: samples

Projects

None yet

Milestone

3.4.11

Development

Successfully merging this pull request may close these issues.

5 participants

@vnikoofard @asmorkalov @sergregory @opencv-pushbot @alalek