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

Number Comparator #15

Merged
merged 9 commits into from
Mar 3, 2023
Merged

Conversation

MajorLift
Copy link
Collaborator

@MajorLift MajorLift commented Feb 8, 2023

  1. Implements Comparator for natural-numbers, signed integers and decimals.
  • decimalCompare is only intended to compare user-inputted exact decimal values, not binary representations with rounding.
  • TODO:
    • resolve "ts(2589) type instantiation is excessively deep" error for test case 42.04, 42.02.
  • Roadmap:
    • Support scientific notation, bin(0b), oct(0o), hex(0x)
    • Fractional module for exact representation of fractions.
    • Decimal and DecimalDigitList modules.
  1. Also implements
  • digit-list from-string method
    • exposes _$fromString2 method which doesn't trim leading zeros.
    // ["4", "2"]
    $<DigitList.FromString, "042">
  • number absolute method
    • converts sign of any input number to positive.
    // 42.42
    $<Number.Absolute, -42.42>

@poteat
Copy link
Owner

poteat commented Feb 9, 2023

I think there are some very valuable contributions here, and I'm intrigued about your approach.

I am concerned about performance - I notice that you are performing subtraction internally in some substeps. To me, comparison should be 'strictly simpler' than subtraction, even for decimal numbers.

Re ontology, I am drawn to potential future modules DecimalDigitList and Decimal, e.g. Decimal.Compare. To me, Fractional would be a hypothetical module that performs arithmetic by representing numbers as fractions, i.e. would be capable of exact representation of 1 / 3. That doesn't seem to be the case here, however.

By encoding core arithmetic logic in DecimalDigitList, I believe we could implement a more performant version, and avoid excessive conversion between numeric and string-list representation.

Finally, re your two failed test cases, I have a suspicion that this might be related to an interesting bit of nuance I found the other day re the current DigitList.Subtract implementation.

@MajorLift
Copy link
Collaborator Author

MajorLift commented Feb 10, 2023

I definitely agree with having distinct Decimal and Fractional modules. How to handle exact values vs. approximate representations was the question I had going into this, and this PR is only intended to cover the first use case.

I rewrote the _$decimalCompare method to avoid using subtract. I believe this implementation should be more performant. The excessive depth error hasn't gone away, however, although one of the two test cases previously affected isn't anymore (error for 42.04, 42.02 but not 42.02, 42.04). Would love to learn more about the nuance you found!


  • Possible bug in Boolean._$and, Conditional._$equals, or Digit._$compare?
// works
  A_DONE extends boolean = Conditional._$equals<A, []>,
  B_DONE extends boolean = Conditional._$equals<B, []>,
  RESULT extends 1 | 0 | -1 = 
    Boolean._$and<A_DONE, B_DONE> extends true
// error
  RESULT extends 1 | 0 | -1 = 
    Boolean._$and<Conditional._$equals<A, []>, Conditional._$equals<B, []>> extends true
    // also error 
    List._$every<$<Conditional.Extends, []>, [A, B]> extends true

error TS2344: Type '0 | 1 | -1' does not satisfy the constraint '1 & _'.
  Type '0' is not assignable to type '1 & _'.
    Type '0' is not assignable to type '1'.

@MajorLift MajorLift force-pushed the 230207-compare-fractions branch 3 times, most recently from c0e3707 to da94fcb Compare March 2, 2023 07:46
- exposes `_$fromString2` method which doesn't trim leading zeros.
- SUPERSEDED by subsequent commit
- TODO: resolve "ts(2589) type instantiation is excessively deep" error for test case `42.04, 42.02`.
- Simpler implementation for `_$decimalCompare` that doesn't rely on `DigitList.Subtract` and should have less performance overhead.
- "Type instantiation is excessively deep..." error persists for test case `42.04, 42.02`, but no longer occurs for `42.02, 42.04`.

- Anomaly: Replacing `A_DONE, B_DONE` in line 43 with their defaults from lines 39-40 results in the following error. Possible bug in `Boolean._$and` or `Conditional._$equals`?
```
error TS2344: Type '0 | 1 | -1' does not satisfy the constraint '1 & _'.
  Type '0' is not assignable to type '1 & _'.
    Type '0' is not assignable to type '1'.
```
@poteat
Copy link
Owner

poteat commented Mar 2, 2023

I see you're still working on this (and you may have fixed the only remaining issue while I've been investigating), but I've noticed that there is no as such infinite loop: rather, it's just returning never:
image
Because it was the only way I figured out how to cause an error, if you pass never into Test.Expect it causes an infinite loop. This is because otherwise since never is a universal subtype, I could "never" cause it not to otherwise pass.

@poteat
Copy link
Owner

poteat commented Mar 2, 2023

This strategy I use sometimes for debugging of complicated, multistage types may prove useful to you:
image

@MajorLift
Copy link
Collaborator Author

This strategy I use sometimes for debugging of complicated, multistage types may prove useful to you:

That is a fantastic idea. Can't believe I never thought to try that.

@poteat
Copy link
Owner

poteat commented Mar 2, 2023

I suspect you will find this in short order. I have gotten all tests to pass on my side. Check out my feature/compare-fractions-testing branch on this repo if you want to see the solution :D

- Returning `never` instead of accumulator caused errors in decimal compare implementation
@MajorLift MajorLift marked this pull request as ready for review March 2, 2023 08:31
@MajorLift
Copy link
Collaborator Author

MajorLift commented Mar 2, 2023

Wow that turned out to be a really simple fix. Thanks for the sanity-restoring debugging tips.😂

@poteat poteat merged commit 985da1f into poteat:main Mar 3, 2023
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.

None yet

2 participants