Skip to content

overload 2 arg show for probs and sols#465

Open
oscardssmith wants to merge 2 commits into
SciML:masterfrom
oscardssmith:pretty-show-probs-and-sols
Open

overload 2 arg show for probs and sols#465
oscardssmith wants to merge 2 commits into
SciML:masterfrom
oscardssmith:pretty-show-probs-and-sols

Conversation

@oscardssmith

Copy link
Copy Markdown
Member

@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

codecov Bot commented Jul 13, 2023

Copy link
Copy Markdown

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

type_color, isinplace(prob),
no_color)
end
Base.show(io::IO, A::AbstractSciMLProblem) = show(io, MIME"text/plain", A)

Copy link
Copy Markdown
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
Copy Markdown
Member 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
Copy Markdown
Member

So this should close?

@oscardssmith

Copy link
Copy Markdown
Member 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