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

overload 2 arg show for probs and sols #465

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

oscardssmith
Copy link
Contributor

@ChrisRackauckas thinks this might break things, but I want to appeal to CI. This makes it so SCIMLProblems and soltuions print nicely when they are within other structs.

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #465 (d3dbde4) into master (f2eab6b) will increase coverage by 7.97%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
+ Coverage   43.47%   51.45%   +7.97%     
==========================================
  Files          47       47              
  Lines        3558     3564       +6     
==========================================
+ Hits         1547     1834     +287     
+ Misses       2011     1730     -281     
Impacted Files Coverage Δ
src/problems/problem_utils.jl 23.33% <0.00%> (+12.09%) ⬆️
src/solutions/solution_interface.jl 54.45% <0.00%> (+25.37%) ⬆️

... and 28 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -39,6 +39,7 @@ function Base.summary(io::IO, prob::AbstractLinearProblem)
type_color, isinplace(prob),
no_color)
end
Base.show(io::IO, A::AbstractSciMLProblem) = show(io, MIME"text/plain", A)
Copy link
Member

Choose a reason for hiding this comment

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

I think this pretty much goes against the default and expected behaviour of show, see eg https://docs.julialang.org/en/v1/manual/types/#man-custom-pretty-printing. The two args method should print a compact one-line representation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. The 2 arg version shouldn't have the line breaks. I'll take another pass on this tomorrow (that also removes the redundancy of all the show methods.)

@ChrisRackauckas
Copy link
Member

So this should close?

@oscardssmith
Copy link
Contributor Author

No. I'm planning on updating this today so that the 2 argument version prints a compact view (but moved onto 1 line).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants