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

Improve error message for non-broadcasted addition and subtraction #3558

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

odow
Copy link
Member

@odow odow commented Nov 3, 2023

Closes #3554

Follow-up to #3557

cc @jd-foster thoughts on this more general error?

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (944e5c5) 98.37% compared to head (3518959) 98.38%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3558   +/-   ##
=======================================
  Coverage   98.37%   98.38%           
=======================================
  Files          37       37           
  Lines        5610     5624   +14     
=======================================
+ Hits         5519     5533   +14     
  Misses         91       91           
Files Coverage Δ
src/nlp_expr.jl 99.52% <100.00%> (+<0.01%) ⬆️
src/operators.jl 97.07% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jd-foster
Copy link
Collaborator

Looks much more general. I wonder about the related case that also prompted this, #3553, that it might not be obvious that x * LinearAlgebra.I == 1.0 * x. I'm not sure this can be covered here, but might be documented somewhere along the lines of the deleted comment "if you are modifying the diagonal entries of the matrix, do A - x * LinearAlgebra.I(n), where n is the diagonal length."

@odow
Copy link
Member Author

odow commented Nov 3, 2023

I think in retrospect that supporting UniformScaling was the wrong choice. But yeah, I don't have a good answer for where we should document this. Putting it in an error message that probably happens when the user didn't write I isn't great either.

@blegat
Copy link
Member

blegat commented Nov 3, 2023

We can restrict the error message to square matrices. I think it's still nice to include it. It shows why it's ambiguous to add a scalar and a matrix because both would be reasonable interpretations

@odow
Copy link
Member Author

odow commented Nov 5, 2023

How's this

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

I like it

@odow odow merged commit ec3c779 into master Nov 6, 2023
11 checks passed
@odow odow deleted the od/fix-error-message branch November 6, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Provide better error message for Vector+VariableRef
3 participants