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

Mixed unit list conversion #568

Merged
merged 13 commits into from
Oct 8, 2024
Merged

Conversation

Goju-Ryu
Copy link
Contributor

@Goju-Ryu Goju-Ryu commented Sep 17, 2024

This PR adds a function that converts a value into a list of values with differing units. It aims to provide the functionality requested in #364, in a way that resembles the inspiration from GNU but with actual values and units, in contrast to the earlier string based implementations in #501.

It currently can not be used with the to keyword or the -> operators as it requires two arguments. I do plan to fix this by updating it to a curried version when/if closures (#347) are implemented.

example:

12 m + 34 cm + 5 mm + 678 µm |> unit_list([m, cm, mm])
#  = [12 m, 34 cm, 5.678 mm]    [List<Length>]

@triallax
Copy link
Contributor

this would require modifying rust code, and i'm not sure how good of an idea it is, but implementing this functionality like so would be pretty cool:

12 m + 34 cm + 5 mm + 678 µm -> [m, cm, mm]
#  = [12 m, 34 cm, 5.678 mm]    [List<Length>]

@Goju-Ryu
Copy link
Contributor Author

this would require modifying rust code, and i'm not sure how good of an idea it is, but implementing this functionality like so would be pretty cool:

12 m + 34 cm + 5 mm + 678 µm -> [m, cm, mm]
#  = [12 m, 34 cm, 5.678 mm]    [List<Length>]

That would be a really cool syntax for it, though I would need help with it as I have no experience with rust. If @sharkdp approves the addition of this syntax, I would love for it to happen though.

@sharkdp
Copy link
Owner

sharkdp commented Sep 24, 2024

Thank you very much! I'll review this soon.

@Goju-Ryu Goju-Ryu force-pushed the mixed-units-conversion branch from f891b3e to 875a0ad Compare September 25, 2024 19:03
@sharkdp
Copy link
Owner

sharkdp commented Sep 28, 2024

I like this very much. Can we please re-implement the existing functions DMS, DM, feet_and_inches and pounds_and_ounces in terms of your unit_list function and get rid of the existing String-based _mixed_units_helper/_mixed_units helpers?

Concerning special syntax for this: I definitely think it is worth thinking about, but let's please shift this to another ticket/PR. Another thing that we can discuss is a special display mode for unit list results. This is part of a more general topic I've been thinking about. It would be great if we had something like a Display trait in Rust that could be implemented for custom types. This way, the unit_list function could return a struct UnitList<T> { parts: List<T> } (once we have #452). And then we could write a Display implementation for UnitList<T> that would render the it as 3 ft + 2 inch instead of [3 ft, 2 inch]. This would look nice, without having to resort to the String hack that we currently use on master.

@Goju-Ryu
Copy link
Contributor Author

I like this very much. Can we please re-implement the existing functions DMS, DM, feet_and_inches and pounds_and_ounces in terms of your unit_list function and get rid of the existing String-based _mixed_units_helper/_mixed_units helpers?

Sure, no problem.

Concerning special syntax for this: I definitely think it is worth thinking about, but let's please shift this to another ticket/PR. Another thing that we can discuss is a special display mode for unit list results. This is part of a more general topic I've been thinking about. It would be great if we had something like a Display trait in Rust that could be implemented for custom types. This way, the unit_list function could return a struct UnitList<T> { parts: List<T> } (once we have #452). And then we could write a Display implementation for UnitList<T> that would render the it as 3 ft + 2 inch instead of [3 ft, 2 inch]. This would look nice, without having to resort to the String hack that we currently use on master.

Agreed, it is definitely outside the scope of this PR. I really like your idea for the implementation too, I could imagine it being a nice way of implementing the human function too if implemented as a mix of a unit_list and Display trait. But I'll leave that for Issues related to the topic.

Since @irevoire fixed problems arrising from calls to simplify, a lot of rounding/conversion bugs have been solved by extension. This lets the function work with a far simpler implementation.
@Goju-Ryu Goju-Ryu force-pushed the mixed-units-conversion branch from 875a0ad to 136114f Compare September 29, 2024 17:50
@sharkdp
Copy link
Owner

sharkdp commented Sep 29, 2024

I think the (Numbat-based) tests for DMS/DM/… need some adaptations from the string-based format to the list-based format.

@Goju-Ryu
Copy link
Contributor Author

Yeah I completely forgot about that, but I'll fix that now

@Goju-Ryu
Copy link
Contributor Author

I had to assert using strings to overcome the problems inherent with comparing floats. This did make me realize that it would be great to be able to provide a margin of error when comparing lists as well. Obviously not something for this PR, but something that would make testing list functions easier in the future.

@Goju-Ryu
Copy link
Contributor Author

I can't figure out the problem with the documentation. I try to run the book/build.py script and get this error

C:\Users\...\numbat\book\src\example-voyager.md
Traceback (most recent call last):
  File "C:\Users\...\numbat\book\build.py", line 64, in <module>
    generate_example("voyager", "Voyager")
  File "C:\Users\...\numbat\book\build.py", line 19, in generate_example
    for line in fin:
                ^^^
  File "C:\Users\...\AppData\Local\Programs\Python\Python312\Lib\encodings\cp1252.py", line 23, in decode
    return codecs.charmap_decode(input,self.errors,decoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in position 391: character maps to <undefined>

I can't figure out why it fails now, as I haven't touched that file. Any help with this would be greatly appreciated.

@sharkdp
Copy link
Owner

sharkdp commented Sep 29, 2024

Oh, it looks like you don't use utf-8 as a default system encoding (but cp1252 instead, similar to Latin-1). Can you please try to add a encoding="utf-8" argument to all open(…) calls in build.py? As in:

with open(path_in, "r", encoding="utf-8") as fin:

If that works, please commit that change to your branch.

@Goju-Ryu
Copy link
Contributor Author

Goju-Ryu commented Sep 29, 2024

This definitely helped, but I get a new error. It seems to be a missing dependency or similar.

Generating list of functions for module 'extra::color'...
Traceback (most recent call last):
  File "C:\Users\...\numbat\book\build.py", line 266, in <module>
    subprocess.run(["mdbook", "build"], text=True)
  File "C:\Users\...\AppData\Local\Programs\Python\Python312\Lib\subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\...\AppData\Local\Programs\Python\Python312\Lib\subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Users\...\AppData\Local\Programs\Python\Python312\Lib\subprocess.py", line 1538, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [WinError 2] The system cannot find the file specified

@sharkdp
Copy link
Owner

sharkdp commented Sep 29, 2024

Oh yes, sorry. You need mdbook installed (https://rust-lang.github.io/mdBook/guide/installation.html). We should improve the error message there.

Please let me know if I should simply generate the documentation for you.

@Goju-Ryu
Copy link
Contributor Author

Oh yes, sorry. You need mdbook installed (https://rust-lang.github.io/mdBook/guide/installation.html). We should improve the error message there.

Ahh, of course. I'll install that and try again. Thank you for all the help.

Please let me know if I should simply generate the documentation for you.

No need, I would like to have it work so I can do it myself in future contributions.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2024

Thank you very much for the updates. I added an @example for unit_list (something that became possible recently), fixed how DMS/DM were displayed, and replaced foldl(_add, 0) by the sum function.

@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2024

One thing I thought about was whether the implementation of the human function could profit from unit_list. Maybe worth checking out.

@sharkdp sharkdp merged commit ced476b into sharkdp:master Oct 8, 2024
15 checks passed
@Goju-Ryu
Copy link
Contributor Author

Goju-Ryu commented Oct 9, 2024

One thing I thought about was whether the implementation of the human function could profit from unit_list. Maybe worth checking out.

I would not be surprised if a lot of logic could be simplified there. I'll look into it and see what I can do.

@Goju-Ryu Goju-Ryu deleted the mixed-units-conversion branch October 23, 2024 19:47
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