Refactoring and minor bug fixing in requirements fixer #1760
Conversation
Summary: The requirements-fixing code is not easy to understand, and some small errors and inefficiencies have crept in. I'm going to fix them in this diff to make it then easier to add a broadcasting fixer in an upcoming diff. In this diff: * elementwise multiplication now correctly creates the requirement that the input types be identical with the output type. (As noted, in an upcoming diff we'll take care of the situation where inputs of dissimilar shape can be fixed by broadcasting.) * Fixed a typo with a misspelling of "requirements" * Methods `_meet_real_matrix_requirement_type`, and `_meet_real_matrix_requirement` were misordered and redundant to other logic in the operator requirements fixer. By more carefully ordering the cases in the operator requirements fixer, we can simply delete these redundant methods. * As noted, `_meet_operator_requirement` was too long and confusing. I've broken it up into many smaller cases each with its own logic for applying a simple fix. * Making these changes had the beneficial side effect of fixing a very small bug in a test case. In test_fix_vectorized_models_8 we have a multiplication of a matrix of probabilities and a constant matrix with all positive real values. The previous code converted both operands to a real matrix, but we can legally convert them to positive real matrix, a more specific type. We now do so. Reviewed By: AishwaryaSivaraman Differential Revision: D40459965 fbshipit-source-id: 6da742006341fef204d9fd9fc81a723e145b98b6
facebook-github-bot
added
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D40459965 |
|
Hi @ericlippert! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Summary:
The requirements-fixing code is not easy to understand, and some small errors and inefficiencies have crept in. I'm going to fix them in this diff to make it then easier to add a broadcasting fixer in an upcoming diff.
In this diff:
elementwise multiplication now correctly creates the requirement that the input types be identical with the output type. (As noted, in an upcoming diff we'll take care of the situation where inputs of dissimilar shape can be fixed by broadcasting.)
Fixed a typo with a misspelling of "requirements"
Methods
_meet_real_matrix_requirement_type, and_meet_real_matrix_requirementwere misordered and redundant to other logic in the operator requirements fixer. By more carefully ordering the cases in the operator requirements fixer, we can simply delete these redundant methods.As noted,
_meet_operator_requirementwas too long and confusing. I've broken it up into many smaller cases each with its own logic for applying a simple fix.Making these changes had the beneficial side effect of fixing a very small bug in a test case. In test_fix_vectorized_models_8 we have a multiplication of a matrix of probabilities and a constant matrix with all positive real values. The previous code converted both operands to a real matrix, but we can legally convert them to positive real matrix, a more specific type. We now do so.
Reviewed By: AishwaryaSivaraman
Differential Revision: D40459965