Skip to content

Comments

Allow old-style shape in blobproto_to_array#3200

Merged
jeffdonahue merged 1 commit intoBVLC:masterfrom
lukeyeager:bvlc/fix-blobproto_to_array
Oct 15, 2015
Merged

Allow old-style shape in blobproto_to_array#3200
jeffdonahue merged 1 commit intoBVLC:masterfrom
lukeyeager:bvlc/fix-blobproto_to_array

Conversation

@lukeyeager
Copy link
Contributor

Fixes #3199
Bug introduced in #3170

@jeffdonahue
Copy link
Contributor

Thanks @lukeyeager, sorry for neglecting to think about this in #3170. I think the logic should be reversed though -- i.e., should check if any of the legacy fields are present and use them if so, othereise use the shape. This way, a BlobProto with neither the legacy fields nor a shape will be interpreted as having an empty shape (a scalar). This is consistent with the behavior of Blob::FromProto.

@lukeyeager lukeyeager force-pushed the bvlc/fix-blobproto_to_array branch from 13503f7 to 4376d17 Compare October 15, 2015 19:26
@lukeyeager
Copy link
Contributor Author

I've updated the logic and added a third test.

Copy link
Contributor

Choose a reason for hiding this comment

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

This wasn't quite what I meant -- this will give a 1D array, not a 0D array (scalar). You'd still want to return data.reshape(blob.shape.dim), or equivalently return data.reshape(()). The invariant we want is len(data.shape) == len(blob.shape.dim) (unless any of the legacy fields are present). It will only work in the case where len(data) == 1, but that's okay since a BlobProto with no shape field (and no legacy dims) is kind of an odd corner case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I'm not letting it fall back to this worthless error:

  File "/home/tranlaman/BLVC-caffe/python/caffe/io.py", line 26, in blobproto_to_array
    return np.array(blob.data).reshape(*blob.shape.dim)
TypeError: function takes exactly 1 argument (0 given)

How about raising a ValueError?

          if len(data) != 1:
              raise ValueError('blob has no shape')
          # Return scalar
          return data[0]

Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove the * (reshape(blob.shape.dim)) it should work? ValueError seems okay to me too, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, that works. Good call.

@lukeyeager lukeyeager force-pushed the bvlc/fix-blobproto_to_array branch from 4376d17 to 75e859a Compare October 15, 2015 20:57
@lukeyeager
Copy link
Contributor Author

Updated logic again and added a fourth test.

@jeffdonahue
Copy link
Contributor

Thanks for the fixes and tests @lukeyeager! LGTM.

jeffdonahue added a commit that referenced this pull request Oct 15, 2015
Allow old-style shape in blobproto_to_array
@jeffdonahue jeffdonahue merged commit 6232dfc into BVLC:master Oct 15, 2015
@lukeyeager lukeyeager deleted the bvlc/fix-blobproto_to_array branch October 15, 2015 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants