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

Truncate printing of large expressions #3575

Merged
merged 10 commits into from
Nov 27, 2023
Merged

Truncate printing of large expressions #3575

merged 10 commits into from
Nov 27, 2023

Conversation

odow
Copy link
Member

@odow odow commented Nov 16, 2023

Closes #3574

I don't know if this is the right approach, but we should do something.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (283a99d) 98.38% compared to head (22bd2a4) 98.38%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3575   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files          37       38    +1     
  Lines        5629     5635    +6     
=======================================
+ Hits         5538     5544    +6     
  Misses         91       91           

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

@pulsipher
Copy link
Contributor

It would be nice to also truncate terms in flattened nonlinear sums. Should be straightforward to add for expressions of the form: NonlinearExpr(:+, [nlp_sum_terms...]).

@odow
Copy link
Member Author

odow commented Nov 16, 2023

Truncated the nonlinear stuff.

My main hesitation is the use of TERM_LIMIT_FOR_PRINTING as a global constant. But maybe that's okay. It's really a debugging flag. Maybe we should just document that extensions should not modify this, leave it to the user. And that extensions should never rely on the output of function_string remaining stable.

@pulsipher
Copy link
Contributor

pulsipher commented Nov 16, 2023

My main hesitation is the use of TERM_LIMIT_FOR_PRINTING as a global constant. But maybe that's okay. It's really a debugging flag. Maybe we should just document that extensions should not modify this, leave it to the user. And that extensions should never rely on the output of function_string remaining stable.

This seems reasonable from an extension point of view, so long as function_string doesn't have significant changes that impact testing frequently. In this case, having the default term limit be 60 is plenty large and likely will not impact any testing that relies on printing. For InfiniteOpt, I don't think this will break any tests since the string checks are for small expressions.

@odow odow requested a review from blegat November 20, 2023 19:29
@odow
Copy link
Member Author

odow commented Nov 20, 2023

@blegat thoughts?

@blegat
Copy link
Member

blegat commented Nov 21, 2023

I think it's yet another proof that we should unify printing of JuMP and MOI. In MOI we have _PrintOptions and this would be a good candidate of additional option for _PrintOption. This would be much cleaner than modifying a global constant.

@odow
Copy link
Member Author

odow commented Nov 21, 2023

Should or shouldn't?

@blegat
Copy link
Member

blegat commented Nov 21, 2023

Indeed, I meant should

@odow
Copy link
Member Author

odow commented Nov 21, 2023

Should we make it a private constant for now then? Merge this PR, and open an issue to unify the printing?

@blegat
Copy link
Member

blegat commented Nov 22, 2023

Yes, if it's private then I don't have any objection to merge this

@odow
Copy link
Member Author

odow commented Nov 26, 2023

Made it a private constant

test/test_print.jl Outdated Show resolved Hide resolved
test/test_print.jl Outdated Show resolved Hide resolved
test/test_print.jl Outdated Show resolved Hide resolved
@odow
Copy link
Member Author

odow commented Nov 27, 2023

@odow odow mentioned this pull request Nov 27, 2023
17 tasks
@odow odow merged commit 96b4147 into master Nov 27, 2023
24 checks passed
@odow odow deleted the od/truncate-printing branch November 27, 2023 21:35
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.

Truncate printing expressions if they are very large
3 participants