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

Reversible uparse method and string method #569

Closed
wants to merge 0 commits into from
Closed

Reversible uparse method and string method #569

wants to merge 0 commits into from

Conversation

michikawa07
Copy link
Contributor

@michikawa07 michikawa07 commented Oct 2, 2022

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
Copy link
Contributor Author

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

@sostock
Copy link
Collaborator

sostock commented Oct 31, 2022

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 #214 (comment)

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).

Copy link
Collaborator

@sostock sostock left a comment

Choose a reason for hiding this comment

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

Just two small comments for now (regarding code style)

src/display.jl Outdated
end
sortexp(::Dimensions{D}) where D = sortexp(D)
sortexp(::Units{U}) where U = sortexp(U)
sortexp(xs)= sort!(collect(xs), by = u->power(u)>0 ? 1 : -1, rev=true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
sortexp(xs)= sort!(collect(xs), by = u->power(u)>0 ? 1 : -1, rev=true)
sortexp(xs)= sort!(collect(xs), by = u->power(u)<0)

This seems easier to me, although in practice it doesn’t matter performance-wise (since xs is typically very short).

Copy link
Contributor Author

@michikawa07 michikawa07 Oct 31, 2022

Choose a reason for hiding this comment

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

Thank you for your reading.

You make a very valid point, but I intentionally wrote it to match the behavior of the existing sortexp as follows.

function sortexp(xs)
   vcat([x for x in xs if power(x) >= 0],
        [x for x in xs if power(x) < 0])
end

So, I think these are better
sortexp(xs)= sort!(collect(xs), by = u->power(u)>0 ? 1 : -1, rev=true)
or
sortexp(xs)= (collect(xs), by = u->power(u)<=0 ? -1 : 1)
than
sortexp(xs)= (collect(xs), by = u->power(u)<0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The original sortexp doesn’t refer to 1 or -1, so I don’t see what it has to do with that. How does using 1 and -1 instead of true and false match the original sortexp better?

src/string.jl Outdated Show resolved Hide resolved
@michikawa07
Copy link
Contributor Author

michikawa07 commented Oct 31, 2022

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.

@sostock
Copy link
Collaborator

sostock commented Oct 31, 2022

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.

@michikawa07
Copy link
Contributor Author

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

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.

2 participants