-
Notifications
You must be signed in to change notification settings - Fork 65
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
fix for 212 (WIP) #213
base: dev
Are you sure you want to change the base?
fix for 212 (WIP) #213
Conversation
I managed to get it to "work" in a somewhat convoluted way, but i still have some new test-cases that fail. they fail also in the current dev branch, though, e.g. There seem to be some logic in place directly for enforcing this behavior (or a related one) but I cannot figure out what's behind it. The comment (in |
sorry I have been quite busy this week and will maybe get to have a look I the coming days. |
no problem at all, we are all busy :)
…On Thu, Jan 12, 2023, 20:27 Niels Mündler ***@***.***> wrote:
sorry I have been quite busy this week and will maybe get to have a look I
the coming days.
—
Reply to this email directly, view it on GitHub
<#213 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADRLNBDET7ETH7JOHWPX6TWSBEKNANCNFSM6AAAAAATUCJF4A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Finally got around to look at this. I think one issue when I picked up this project was that something like "kgm" would not be picked up as "kilogram meters" - because considering space a multiplication factor caused a lot of issues. One way to cope with this was that I just fixed "either all multiplications are explicit or all are implicit" - the way normal humans would write units. Something like "gml" (gramm meter litres) is super awkward - either it would be "gml" or "gm*l". Does that help you in any way? I am wondering if the "different results" stem alone from the fact that operators are stored in sets and the order in which they are accessed may depend on their runtime hash ordering in the set - which might change on every reload of quantulum. This might be simply fixed by usings lists instead of sets to store them. |
thanks,
Yes, this makes sense!
But I do think that if an operator is explicit, we should allow it also if
surrounded by space. I will try to make it work.
Re the ordering fix: yes, this is indeed because of sets, and I have a fix
for that in the PR, but unfortunately it fixed things to be consistent on
the wrong result :( and I think that a solution that depends on the order
in the list of operators is too brittle, so looking for something better.
…On Mon, Jan 16, 2023 at 6:33 PM Niels Mündler ***@***.***> wrote:
There seem to be some logic in place directly for enforcing this behavior
(or a related one) but I cannot figure out what's behind it. The comment
(in parser.get_unit()) says "cut if inconsistent multiplication operator",
can you elaborate on this notion of consistency?
Finally got around to look at this. I think one issue when I picked up
this project was that something like "kgm" would not be picked up as
"kilogram meters" - because considering space a multiplication factor
caused a lot of issues. One way to cope with this was that I just fixed
"either all multiplications are explicit or all are implicit" - the way
normal humans would write units. Something like "gm*l" (gramm meter
litres) is super awkward - either it would be "gml" or "g*m*l". Does that
help you in any way?
I am wondering if the "different results" stem alone from the fact that
operators are stored in sets and the order in which they are accessed may
depend on their runtime hash ordering in the set - which might change on
every reload of quantulum. This might be simply fixed by usings lists
instead of sets to store them.
—
Reply to this email directly, view it on GitHub
<#213 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADRLNCHSX4FMV7LI7XCRZDWSVZ37ANCNFSM6AAAAAATUCJF4A>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
This is an attempt to fix #212
Now the parsing behavior is consistent, but we cannot parse correctly some of the new test examples.