Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split ONNXParser from MarabouONNXNetwork in preparation for replacing with C++ implementation #745

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

MatthewDaggitt
Copy link
Collaborator

This commit starts to lay the ground work for #727 by extracting the parsing components of MarabouONNXNetwork and MarabouNetwork into two new classes ONNXParser and InputQueryBuilder. These two new classes will in a future PR be directly implemented by the existing ONNXParser.h and NetworkParser.h on the C++ side (the latter to be renamed to InputQueryBuilder at later stage).

In particular, I've also changed ONNXParser.py to be a pure operation, simply adding constraints to an InputQueryBuilder, rather than storing the constraints internally. This will greatly ease the later implementation of support for queries over multiple networks.

I've striven to maximise backwards compatibility as much as possible, and I haven't broken any example code. The only two unavoidable changes were as follows:

  1. MarabouONNXNetwork no longer exposes the fields madeGraphEquations, varMap, constantMap, shapeMap.
    This I think is a distinct improvement as these are really internal implementation details of the parser, and have nothing to do with the constraints generated.
  2. MarabouONNXNetwork no longer has a shallowCopy method. Instead of calling this method,
    there's a new parameter preserveExistingConstraints in the method readONNX to
    True which has the same effect and is much more semantically meaningful I think!

maraboupy/Marabou.py Show resolved Hide resolved
maraboupy/MarabouNetwork.py Outdated Show resolved Hide resolved
maraboupy/parsers/InputQueryBuilder.py Show resolved Hide resolved
maraboupy/parsers/InputQueryBuilder.py Outdated Show resolved Hide resolved
@wu-haoze
Copy link
Collaborator

wu-haoze commented Feb 16, 2024

Another general thought is that we could just create a MarabouCore.InputQuery object at the initialization of InputQueryBuilder(), and directly encode bounds, equations, ReLUs into the InputQuery, as the methods in the InputQueryBuilder() class are called.

This is probably beyond the scope of this PR, but would be necessary when we replace in the C++ ONNX parser later on. Because the C++ ONNX parser will create a marabouCore.InputQuery object.

@MatthewDaggitt Let me know what you think! I'm happy to do this refactoring, while you're working on the C++ ONNX end.

maraboupy/MarabouNetwork.py Outdated Show resolved Hide resolved
@MatthewDaggitt
Copy link
Collaborator Author

Another general thought is that we could just create a MarabouCore.InputQuery object at the initialization of InputQueryBuilder(), and directly encode bounds, equations, ReLUs into the InputQuery, as the methods in the InputQueryBuilder() class are called.

This is probably beyond the scope of this PR, but would be necessary when we replace in the C++ ONNX parser later on. Because the C++ ONNX parser will create a marabouCore.InputQuery object.

Yes, eventually I think we should be unifying InputQuery and InputQueryBuilder!

@wu-haoze
Copy link
Collaborator

Another general thought is that we could just create a MarabouCore.InputQuery object at the initialization of InputQueryBuilder(), and directly encode bounds, equations, ReLUs into the InputQuery, as the methods in the InputQueryBuilder() class are called.
This is probably beyond the scope of this PR, but would be necessary when we replace in the C++ ONNX parser later on. Because the C++ ONNX parser will create a marabouCore.InputQuery object.

Yes, eventually I think we should be unifying InputQuery and InputQueryBuilder!

I think there is a motivation to have an extra layer (InputQueryBuilder) because of the user-friendly python specific methods for adding constraints. But the layer should be made thiner than what it is now..

@MatthewDaggitt
Copy link
Collaborator Author

I think there is a motivation to have an extra layer (InputQueryBuilder) because of the user-friendly python specific methods for adding constraints. But the layer should be made thiner than what it is now..

Yes, okay. I'm definitely planning to unify the C++ and the Python at the InputQueryBuilder layer. After that we can look at it and reassess the situation 👍

@MatthewDaggitt MatthewDaggitt merged commit 73927a8 into master Feb 16, 2024
11 checks passed
@MatthewDaggitt MatthewDaggitt deleted the split-onnx-parser branch February 16, 2024 08:16
wu-haoze pushed a commit that referenced this pull request Mar 4, 2024
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.

None yet

2 participants