-
Notifications
You must be signed in to change notification settings - Fork 122
Reversible uparse method and string method #569
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
Conversation
|
I wouldn’t change the 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 Maybe we should continue work on #470 instead? Another possibility would be keep all output the same by default and to introduce a new |
There was a problem hiding this 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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).
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
|
Thank you for your comment. I generally agree that the behavior of 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. I would like to update the method to change
You are right, it would be better to work with #470 when changing |
|
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. |
|
It be okay, I'm a little busy right now, so it would be great if you could do it. |
Trivial fix:
Sortexpfunction in display.jl has been more efficient.Change 1: The
uparsemethod in user.jl is now able to parse units defined in external modules by default.Change 2: The
uparsemethod in user.jl is now able to parseu_strmacro.Change 3: The
usymfunction is added to@unitmacro and others in user.jl to retrieve unit symbol information that has been deleted so far. (This method is similar to theabbrmethod, but returns asymsymbol rather than anabbrstring 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
stringmethod is a downstream method from theshowmethod, so it does not affect the existing code (at least as far asruntest.jlis concerned).I hope this request will be merged.