Unitful.jl icon indicating copy to clipboard operation
Unitful.jl copied to clipboard

Reversible uparse method and string method

Open michikawa07 opened this issue 3 years ago • 1 comments

Trivial fix: Sortexp function in display.jl has been more efficient. Change 1: The uparse method in user.jl is now able to parse units defined in external modules by default. Change 2: The uparse method in user.jl is now able to parse u_str macro. Change 3: The usym function is added to @unit macro and others in user.jl to retrieve unit symbol information that has been deleted so far. (This method is similar to the abbr method, but returns a sym symbol rather than an abbr string in @unit sym abbr ...) Change 4: Add string(x::Quantity) to return a string of units that julia can parse.

The main purpose of this pull request is Change 4. I was inconvenienced by the fact that once numbers with units (i.e. a quantity), such as 1 N m, is saved in another file, it cannot be reloaded by julia. The main reason for not being able to reload was that julia could not parse whitespace between units as a multiplication product, so this has been improved, such as "1(N*m)" (Other minor adjustments were necessary, which I was able to handle to some extent). The string method is a downstream method from the show method, so it does not affect the existing code (at least as far as runtest.jl is concerned).

I hope this request will be merged.

michikawa07 avatar Oct 02 '22 14:10 michikawa07

I think following issue is related. #214 #388 #391 #412 #435

michikawa07 avatar Oct 02 '22 14:10 michikawa07

I wouldn’t change the string behavior to do this, but rather change Base.show (cf. also #470). The docstring for show says that its output should (if possible) be parsable, while string has no such restriction. For example, cf. the behavior of the Dates stdlib:

julia> using Dates; x = Day(1);

julia> x # non-parsable
1 day

julia> show(x) # prints to stdout, parsable
Day(1)
julia> show(stdout, MIME"text/plain"(), x) # prints to stdout, non-parsable
1 day
julia> print(x) # prints to stdout, non-parsable
1 day
julia> string(x) # returns a string, non-parsable
"1 day"

julia> repr(x) # returns a string, parsable
"Day(1)"

julia> repr(MIME"text/plain"(), x) # returns a string, non-parsable
"1 day"

The two styles should both be implemented in Base.show, i.e., Base.show(io, x) should output the parsable and Base.show(io, ::MIME"text/plain", x) the human-readable (non-parsable) version. This was already proposed in https://github.com/PainterQubits/Unitful.jl/issues/214#issuecomment-714911511

Maybe we should continue work on #470 instead?

Another possibility would be keep all output the same by default and to introduce a new IOContext property. One would then have to call something like repr(1u"m/s", :unitful_parsable=true) to get a parsable string, by default the behavior would stay the same. This would be the most “non-breaking”. But I’m also fine with changing the default show(1m/s) behavior (and thereby also repr(1m/s)), as long as we make sure that the default output in the REPL stays the same (also within containers).

sostock avatar Oct 31 '22 13:10 sostock

Thank you for your comment. I think I understand your points and arguments. (Thanks for the examples.)

I generally agree that the behavior of show(1m/s) should be changed.

However, the reason I defined a new string this time is that the DimensionError message test did not pass when I tried changing the show. (The cause may be that the method showerror use print, and print use show (not ::MIME"text/plain")) I did not have the confidence or time to nondestructively fix the DimensionError message, so I decided to use the above implementation.

I would like to update the method to change show(1m/s) at a later date. (The core algorithm is the same as string(1m/s), so it should be relatively quick.) What do you think?

Maybe we should continue work on #470 instead?

You are right, it would be better to work with #470 when changing show.

michikawa07 avatar Oct 31 '22 14:10 michikawa07

Even if we work on #470 instead, I would like to include the first commit from this PR, since it is a performance improvement and doesn’t change any behavior.

@michikawa07 Would you be okay if I cherry-pick that commit and make a separate PR with it? You can also do it yourself if you like.

sostock avatar Oct 31 '22 14:10 sostock

It be okay, I'm a little busy right now, so it would be great if you could do it.

michikawa07 avatar Oct 31 '22 14:10 michikawa07