[bug report] Floating point exception when width and/or height equals crop_size

[mlapin]

In

src/caffe/data_transformer.cpp

replace

      h_off = Rand() % (height - crop_size);
      w_off = Rand() % (width - crop_size);

with

      h_off = height > crop_size ? Rand() % (height - crop_size) : 0;
      w_off = width > crop_size ? Rand() % (width - crop_size) : 0;

[bhack]

Can you prepare a PULL REQUEST?

[shelhamer]

Is this the same bug fixed in dev by #1149 but still in master?

[bhack]

I think that this want also to assert that crop_size in not > of width and height.

[longjon]

@shelhamer right, this is fixed by #1149. @mlapin, you should be able to use the fix by merging with master or whatever branch you are on. Note that the original code is wrong, and the proposed solution is also wrong.

@shelhamer should we merge the fix also to master? I didn't bother since it seemed like an uncommon thing, but maybe this codepath gets hit more often with the data transformer refactoring...

[shelhamer]

Yeah, we should likely cherry-pick the fix. It'll cause a conflict in the
next release merge but oh well.

It's not yet second-nature to merge fixes to master instead, so I've fallen
into the same trap with a few recent fixes in dev.

On Wednesday, October 1, 2014, longjon notifications@github.com wrote:

@shelhamer https://github.com/shelhamer right, this is fixed by #1149
#1149. @mlapin
https://github.com/mlapin, you should be able to use the fix by merging
with master or whatever branch you are on. Note that the original code is
wrong, and the proposed solution is also wrong.

@shelhamer https://github.com/shelhamer should we merge this fix also
to master? I didn't bother since it seemed like an uncommon thing, but
maybe this codepath gets hit more often with the data transformer
refactoring...

—
Reply to this email directly or view it on GitHub
#1199 (comment).

[longjon]

If it's a clean merge to master, can we not simply merge to master as well?

On Wed, Oct 1, 2014 at 11:13 AM, Evan Shelhamer notifications@github.com
wrote:

Yeah, we should likely cherry-pick the fix. It'll cause a conflict in the
next release merge but oh well.

It's not yet second-nature to merge fixes to master instead, so I've
fallen
into the same trap with a few recent fixes in dev.

On Wednesday, October 1, 2014, longjon notifications@github.com wrote:

@shelhamer https://github.com/shelhamer right, this is fixed by #1149
#1149. @mlapin
https://github.com/mlapin, you should be able to use the fix by
merging
with master or whatever branch you are on. Note that the original code
is
wrong, and the proposed solution is also wrong.

@shelhamer https://github.com/shelhamer should we merge this fix also
to master? I didn't bother since it seemed like an uncommon thing, but
maybe this codepath gets hit more often with the data transformer
refactoring...

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

[shelhamer]

The PR was made to dev so you would have to rebase on master or
cherry-pick. If you merge the branch directly it will pull all of dev at
the time of the PR into master along with it.

You can selectively merge from the past to the future, but not from the
future to the past.

On Wed, Oct 1, 2014 at 11:19 AM, longjon notifications@github.com wrote:

If it's a clean merge to master, can we not simply merge to master as
well?

On Wed, Oct 1, 2014 at 11:13 AM, Evan Shelhamer notifications@github.com

wrote:

Yeah, we should likely cherry-pick the fix. It'll cause a conflict in
the
next release merge but oh well.

It's not yet second-nature to merge fixes to master instead, so I've
fallen
into the same trap with a few recent fixes in dev.

On Wednesday, October 1, 2014, longjon notifications@github.com
wrote:

@shelhamer https://github.com/shelhamer right, this is fixed by
#1149
#1149. @mlapin
https://github.com/mlapin, you should be able to use the fix by
merging
with master or whatever branch you are on. Note that the original code
is
wrong, and the proposed solution is also wrong.

@shelhamer https://github.com/shelhamer should we merge this fix
also
to master? I didn't bother since it seemed like an uncommon thing, but
maybe this codepath gets hit more often with the data transformer
refactoring...

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

[longjon]

Ah, of course, I keep forgetting about that.

On Wed, Oct 1, 2014 at 11:38 AM, Evan Shelhamer notifications@github.com
wrote:

The PR was made to dev so you would have to rebase on master or
cherry-pick. If you merge the branch directly it will pull all of dev at
the time of the PR into master along with it.

You can selectively merge from the past to the future, but not from the
future to the past.

On Wed, Oct 1, 2014 at 11:19 AM, longjon notifications@github.com
wrote:

If it's a clean merge to master, can we not simply merge to master as
well?

On Wed, Oct 1, 2014 at 11:13 AM, Evan Shelhamer <
notifications@github.com>

wrote:

Yeah, we should likely cherry-pick the fix. It'll cause a conflict in
the
next release merge but oh well.

It's not yet second-nature to merge fixes to master instead, so I've
fallen
into the same trap with a few recent fixes in dev.

On Wednesday, October 1, 2014, longjon notifications@github.com
wrote:

@shelhamer https://github.com/shelhamer right, this is fixed by
#1149
#1149. @mlapin
https://github.com/mlapin, you should be able to use the fix by
merging
with master or whatever branch you are on. Note that the original
code
is
wrong, and the proposed solution is also wrong.

@shelhamer https://github.com/shelhamer should we merge this fix
also
to master? I didn't bother since it seemed like an uncommon thing,
but
maybe this codepath gets hit more often with the data transformer
refactoring...

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

—
Reply to this email directly or view it on GitHub
#1199 (comment).

[shelhamer]

Already fixed.