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

Switched to use posito when testing specialized posit<16,2> #409

Closed

Conversation

davidmallasen
Copy link
Contributor

Following issue #343. Everything seems to be working fine, but I still get different results in my application. Maybe the problem is not in the basic operations but in some other detail.

@davidmallasen davidmallasen changed the base branch from main to v3.76 February 2, 2024 17:50
@ghost ghost requested a review from Ravenwater February 4, 2024 13:59
Copy link
Contributor

@Ravenwater Ravenwater left a comment

Choose a reason for hiding this comment

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

Let's not modify the regression test, but instead let's create a new exploratory test in the posito section of the tree. That way we have much more freedom to experiment and we are not polluting the posit regression test suite.

We also should not need the new posit_specialized_test_suite_randoms.hpp or the test_reporters.hpp modification: The originals should be parameterized properly so that we can throw both posit<> and posito<> types at them.

So, recap:

  • posit_specialized_test_suite_randoms.hpp and test_reporters.hpp mods should be removed
  • back out the changes to posit_16_2.cpp to its original form
  • add a new test posit_16_2.cpp to the posito/arithmetic directory that will contain the code for RCAing any differences between posit specialized and posito.

#include <universal/verification/posit_test_suite.hpp>
#include <universal/verification/posit_test_suite_mathlib.hpp>
#include <universal/verification/posit_test_suite_randoms.hpp>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not modify the regression suite, but instead, let's add a new exploratory test so we can RCA this issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ravenwater I have a few issues with what you ask:

  • How do you propose to do the type generalization in posit_test_suite_randoms.hpp? As it is now it's not straightforward to do, and I'm not sure what your plans are.
  • To keep the test_reporters.hpp as-is, we'd need some sort of conversion between posit and posito. Where should that be added? I guestt it should suffice by copying the bits, but I might be wrong.
  • Why not keep these extra tests in the posit/specialized folder? That's where the tests should be, not in some posito folder since we're not testing for posito. I'm saying this especially because these tests are passing. It also serves as a way of avoiding the issue we had at the beginning where the specialized posit<16,2> was behaving as a posit<16,1> and still passing these tests.

Copy link

Choose a reason for hiding this comment

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

@davidmallasen you are correct, we want to bring in posito<> as an oracle.

Your mod got caught in the work-in-progress of unifying the regression API among all the number systems, which tripped over SFINAE failures. I am going to take your design and merge it into the new API and try to make it do double duty between marshalling through double and using an oracle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! Feel free to use my modifications in this PR in any way you are designing the new API. If at any point this PR is no longer relevant we can close it.

@Ravenwater
Copy link
Contributor

@davidmallasen I got the new api in branch v3.76

But as I was prepping for the merge, I had new thought:
1- randoms are unlikely to catch corner cases
2- we tested the arithmetic operators exhaustively
3-the regression test shows float conversion failures

From those observations, it appears that our RCA between behavioral differences between fast posit<16,2> and posito<16,2> should first resolve the float conversion failures.

@Ravenwater
Copy link
Contributor

@davidmallasen the oracle concept to be a reference type in the regression suites needs more thought. What is the 'right' Oracle type? Will such Oracle types depend on the nature of the type under test?

To research those issues, I will continue to operate under posito<> and explore these issues before jumping into a GoldenType template parameter.

@Ravenwater
Copy link
Contributor

And the saga continues:

posit conversion validation: results only
posit<5,2>                                                   conversion PASS
posit<6,2>                                                   conversion PASS
posit<7,2>                                                   conversion PASS
posit<8,2>                                                   conversion FAIL 484 failed test cases
posito<8,2>                                                  conversion PASS
posit<9,2>                                                   conversion PASS
FAIL = 0.06251519918             did not convert to 0.06253051758             instead it yielded  0.0625                     raw 0b0.01.00.00000000000
FAIL = 0.999878943               did not convert to 0.9997558594              instead it yielded  1                          raw 0b0.10.00.00000000000
posit<16,2>                                                  conversion FAIL 2 failed test cases
FAIL = 0.06251519918             did not convert to 0.06253051758             instead it yielded  0.0625                     raw 0b0.01.00.00000000000
FAIL = 0.999878943               did not convert to 0.9997558594              instead it yielded  1                          raw 0b0.10.00.00000000000
posito<16,2>                                                 conversion FAIL 2 failed test cases
posit conversion validation: FAIL

Looks like there is a bad cast assumption going on in the stack.

@davidmallasen
Copy link
Contributor Author

Hello @Ravenwater. For a generic Oracle type, I don't think the Oracle types should depend on the type under test. For this I would use something like MPFR, although I don't know how complicated it would be to include in Universal. However, in the context of the specialized posit, we do need some way to avoid what happened between posit<16,1> and posit<16,2>, so comparing the bit strings with the stable posit bitblock implementation would still be the way to go.

Thanks for your time on this, and great that you found where the problem is! I didn't get those errors when launching the tests the previous time I don't know why. That could be the problem that I saw where the results were not exactly the same but differed a bit between the specialized/non-specialized versions. Would this be some error in

?

@Ravenwater
Copy link
Contributor

@davidmallasen :-) Nothing to do with either rounding or casting in the posit implementation: it was a bug in the regression test:

posit conversion validation: results only
posit<5,2>                                                   conversion PASS
posit<6,2>                                                   conversion PASS
posit<7,2>                                                   conversion PASS
posit<8,2>                                                   conversion FAIL 484 failed test cases
posito<8,2>                                                  conversion PASS
posit<9,2>                                                   conversion PASS
posit<16,2>                                                  conversion PASS
posito<16,2>                                                 conversion PASS

the specialized posit<8,2> does use a different conversion algorithm for the sake of speed, and that still needs some TLC.

But that leaves still a hole in our theory what to test to RCA the difference between posit<16,2> and specialized posit<16,2> in your application.

Any other ideas? Can you describe the computational path you are seeing differences in?

@Ravenwater
Copy link
Contributor

@davidmallasen and concerning an Oracle type, we are working on a fast Priest arbitrary precision type a la MPFR that could be utilized across Universal. One of the requirements of Universal is that it has no external dependencies, and thus linking in a static library like MPFR is a non-starter.

@davidmallasen
Copy link
Contributor Author

@Ravenwater I'll have to try to pinpoint where the two operations converge. When I get some time I'll try to narrow it down a bit so I can share where the discrepancy is.
I imagined adding MPFR would be a no-go 👍🏽

@Ravenwater Ravenwater deleted the branch stillwater-sc:v3.76 April 27, 2024 01:50
@Ravenwater Ravenwater closed this Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants