Skip to content

Conversation

@progirep
Copy link

This is supposed to address issue 723: #723

There are two changes:

  • Using a non-two-D Convolution Layer now gives an error message
  • If the layer sizes are stored in INT64 instead of INT32, this doesn't lead to memory access violations. The previous behavior was quite weird because when interpreting an INT64 array as an INT32 array, the "-1" at the beginning of the array (in layer sizes) get duplicated, which leads to the reshaping operation code failing in an unexpected way.

The current code is not clean in that probably the error message formatting is not exactly how this is envisioned in the project, the code formatting is not great, and data types are not checked uniformly. The additional checks for data types probably make the overall code a tiny bit slower, but not much.

- Using a non-two-D Convolution Layer now gives an error message
- If the layer sizes are stored in INT64 instead of INT32, this now works
Copy link
Collaborator

@MatthewDaggitt MatthewDaggitt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Generally looks great, but a few comments.

TensorShape inputShape = _shapeMap[inputNodeName];

// Added: Check if convolutional layers are only 2D
if (inputShape.size()!=4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very minor but this should be laid out as:

if ( inputShape.size() != 4 ) 
{

and tabs should be four spaces. Can you check that this style is used everywhere?

result.append( value );
}
} else {
String errorMessage = Stringf( "Illegal data type for integer tensors used in the model. Only INT32 and INT64 supported." );
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be "unsupported" not "illegal"?

} else {
String errorMessage = Stringf( "Illegal data type for integer tensors used in the model. Only INT32 and INT64 supported." );
throw MarabouError( MarabouError::ONNX_PARSER_ERROR, errorMessage.ascii() );
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you also need to add a corresponding case for the else below when the data is not stored in raw_data. I'd advice lifting the dataType check above the raw_data check.

@wu-haoze
Copy link
Collaborator

@progirep Thank you so much for your code contribution. I have two requests (sorry for the trouble)!

  1. Regarding the code style, we do have one that we try to follow and are in the process of strictly enforcing it (Clang format #737). Please feel free to use the .clang-format file to format your PR.

  2. Could you please also squash the commits into a single commit, and commit with the --signoff flag on (i.e., git commit --signoff). This is to affirm that your commit complies with the licensing requirement governing this repo. You might need to stash your changes, recommit, and git push --force.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/managing-the-commit-signoff-policy-for-your-repository#about-commit-signoffs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants