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

Juno COOL printing #13

Merged
merged 5 commits into from
Nov 13, 2017
Merged

Juno COOL printing #13

merged 5 commits into from
Nov 13, 2017

Conversation

Datseris
Copy link
Contributor

No description provided.

@Datseris
Copy link
Contributor Author

Wait don't merge yet. I can improve printing even more!

@giordano
Copy link
Member

Is it really necessary to use an external package to print stuff in Juno? I'd like to avoid adding dependencies just for this :-)

@Datseris
Copy link
Contributor Author

Requires is a 100-lines-of-code pure Julia package that allows you to have code for Juno without having Juno as a dependency. Makes no sense for me to "not want it"...

The way I see it, there are 3 options:

  1. Requires becomes a dependency and we fix it through my PR
  2. You fix it without my PR, by changing the Base.show() method. Juno falls back to show if no method for Juno.render is defined. http://docs.junolab.org/latest/man/info_developer.html#Custom-Type-Printing-1
  3. You don't support Juno which means you lose a significant amount of people using your package. I dont think anyone in the world would be able to use a package that breaks printing, right?

Also, let me say that I find "just for this" a bad thing to say, as many scientists using Julia use/will use Juno and not the REPL! I am a Juno fanatic and felt heart-broken!

@Datseris
Copy link
Contributor Author

How it looks now:
image
and after clicking on the numbers:
image

@giordano
Copy link
Member

giordano commented Nov 13, 2017

Also, let me say that I find "just for this" a bad thing to say, as many scientists using Julia use/will use Juno and not the REPL! I am a Juno fanatic and felt heart-broken!

Don't get me wrong, I really want the issue to be fixed! My point is that the package already defines a working show method for the custom type, which works in the REPL and Jupyter notebooks. It looks to me that there is something "weird" in Juno if it can't handle any custom show method :-)

Option 2 looks the way to go here, I just don't know what's wrong for Juno with the show method already defined. Here is an example of a show method for a custom type, I can't see what's wrong with my definition...

@Datseris
Copy link
Contributor Author

Datseris commented Nov 13, 2017

To be honest with you it is weird for me that you already have a dependency on RecipiesBase and you say you don't want a dependency on Requires. Can you somehow justify this, because as a fellow developer I don't understand it. From my perspective, because it is a pure Julia package and extremely tiny, I wouldn't really care having it as a dependency.

Regardless of what show does, this PR takes advantage of Juno's capabilities and print numbers in a nice way instead of being plain text. If you do not want it go ahead and close the PR.

@Datseris
Copy link
Contributor Author

Also, having cool juno printing would be the icing on the cake! You have support for so many things, like 5 different type of strings that can be made a measurement. Then you just put the cherry on top of this cake and boom!

(I am trying hardcore to convince you because I don't want to clone my own fork and syncrhronise with yours all the time)

@giordano
Copy link
Member

To be honest with you it is weird for me that you already have a dependency on RecipiesBase and you say you don't want a dependency on Requires. Can you somehow justify this, because as a fellow developer I don't understand it. From my perspective, because it is a pure Julia package and extremely tiny, I wouldn't really care having it as a dependency.

I should have expected this question :-D As I already said above, it looked weird to me that one has to define another method for printing stuff only in Juno, when a method for printing stuff elsewhere (REPL, Jupyter) already works.

If something is broken only on a system and works flawless elsewhere, I think it's that system that is broken. It shouldn't be my (or of any other package developer) duty to work around this issue.

Depending on RecipesBases adds a feature, otherwise not available anywhere.

Anyway, we've found that Juno doesn't strictly need a new method and your PR turns from bugfix to a new feature ;-)

Regardless of what show does, this PR takes advantage of Juno's capabilities and print numbers in a nice way instead of being plain text. If you do not want it go ahead and close the issue.

To be honest, I'm not sure I like Juno printing support. It doesn't respect methods for complex number and alignment of array elements defined in the package. Perhaps you can expand the PR to support also them? :-)

@Datseris
Copy link
Contributor Author

Datseris commented Nov 13, 2017

image

To be honest, I'm not sure I like Juno printing support. It doesn't respect methods for complex number and alignment of array elements defined in the package. Perhaps you can expand the PR to support also them? :-)

I don't see anything wrong or not working as expected. And I also didn't write anything new to get this.

Tell me want you want more and I will do it, provided that you will actually merge this PR. Don't have me doing stuff and then say "I guess I don't want them", please.

For example adding the parenthesis for the complex numbers is almost trivial if you want that.

@giordano
Copy link
Member

In the REPL:

julia> complex(rand() ± rand(), rand() ± rand())
(0.25740690768065977 ± 0.4055554063913367) + (0.7851017359568999 ± 0.9717133015119743)im

julia> [10.0^i ± 10.0^-i for i in 0:10]
11-element Array{Measurements.Measurement{Float64},1}:
      1.0±1.0    
     10.0±0.1    
    100.0±0.01   
   1000.0±0.001  
  10000.0±0.0001 
 100000.0±1.0e-5 
    1.0e6±1.0e-6 
    1.0e7±1.0e-7 
    1.0e8±1.0e-8 
    1.0e9±1.0e-9 
   1.0e10±1.0e-10

For the complex numbers there are parentheses around each part, making it more readable and turning the output into valid input (there is also a test to be sure this is always true). Otherwise copying-and-pasting the output is a bit annoying, you have to manually add the parentheses.

I see that Juno doesn't try to align at all standard arrays either, so that's a lost cause.

Tell me want you want more and I will do it, provided that you will actually merge this PR. Don't have doing stuff and then say "I guess I don't want them", please.

Ok, I promise I'll merge the PR if it supports complex numbers as well ;-) You can just adapt the Base.show(io::IO, measure::Complex{<:Measurement}) method, without the compact stuff that Juno doesn't care about. I guess there is no hope to add tests for this without requiring Juno for tests?

As a side note, I hope you see that you have to repeat already valid methods in order to obtain the same feature only in Juno, because it doesn't respect show methods. I wish this isn't necessary, not just for this package, bot for anyone.

@Datseris Datseris changed the title Fix Juno printing breakdown Juno COOL printing Nov 13, 2017
@Datseris
Copy link
Contributor Author

Okay #12 works for me as well now after the update of Juno. I will implement 2 more methods to make this cool and a new feature and then you can merge if you want!

Notice: it is not possible for me to do this without adding the Require package.

Juno doesn't align standard arrays because of the ... thingy where you can click. I can restore the alignment behavior if you want by passing the show method to the render method.

Do you want this? As a Juno user I prefer having the rounded and clickable result.

@codecov-io
Copy link

codecov-io commented Nov 13, 2017

Codecov Report

Merging #13 into master will decrease coverage by 1.55%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage    99.6%   98.04%   -1.56%     
==========================================
  Files           8        8              
  Lines         504      512       +8     
==========================================
  Hits          502      502              
- Misses          2       10       +8
Impacted Files Coverage Δ
src/Measurements.jl 76.47% <0%> (-23.53%) ⬇️
src/math.jl 99.47% <0%> (ø) ⬆️
src/utils.jl 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c8eb8e...c3fa6a1. Read the comment docs.

@Datseris
Copy link
Contributor Author

Pushed method for complex numbers.
Now it looks like this:
image
(copy pasting works if you expand them)

@Datseris
Copy link
Contributor Author

Datseris commented Nov 13, 2017

So the tests fail because of nightlies not because of my changes, apparently.

About adding tests..... I don't see how that is possible without adding Juno, sorry.

I also don't see what could be tested? Whether the print statement returns something expected? Like giving 0.2 +/- 0.1 and seeing if it prints the same?

As a side note, I hope you see that you have to repeat already valid methods in order to obtain the same feature only in Juno, because it doesn't respect show methods. I wish this isn't necessary, not just for this package, bot for anyone.

Yes I saw that I had to repeat 6 lines that compose an if statement, but I do not care about something that small if I get to see numbers and pluses in different colors. Also, all the show methods work perfectly fine in Juno, apparently I had some version mismatches. After updating everything was fine.

@giordano
Copy link
Member

So the tests fail because of nightlies not because of my changes, apparently.

I don't expect your changes to cause a failure in tests ;-) I'm aware of a couple of issues on nightly. One is a bug in Julia, already reported but not yet fixed, another one is the change of mathematical constant. Unless someones needs this to be fixed now I'll address it later.

About adding tests..... I don't see how that is possible without adding Juno, sorry.

Ok, that's fine.

I also don't see what could be tested? Whether the print statement returns something expected? Like giving 0.2 +/- 0.1 and seeing if it prints the same?

Yes, that's what I do for show methods.

@giordano giordano merged commit 7184ca6 into JuliaPhysics:master Nov 13, 2017
@Datseris
Copy link
Contributor Author

Awesome, thanks!

@giordano
Copy link
Member

Thanks to you ;-)

@giordano
Copy link
Member

@Datseris is this feature still working? 🤔 In the README.md of Requires.jl I read that the @require must be in the @__init__ function

@Datseris
Copy link
Contributor Author

Nope I don't think it is valid anymore, exactly because of the reason you said..... I haven't used this in quite a while so I'd suggest you to remove it / comment it out and when I use it again in the future I could make a new PR that fixes everything?

At the moment I am very short on time to do such a thing, sorry.

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.

None yet

3 participants