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.

Sign up for GitHub

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

Jump to bottom

More flexible and precise approx_eq #869

Merged
merged 33 commits into from
Mar 5, 2024
Merged

More flexible and precise approx_eq #869

merged 33 commits into from
Mar 5, 2024

Conversation

bragov4ik
Copy link
Contributor

@bragov4ik bragov4ik commented Jan 16, 2024 •
edited

  • Add tolerances relative to magnitude of the compared numbers (i.e. when error accumulates in a larger computations)
  • Add corresponding macros (to compare by constant (absolute) tolerance, by relative, or by both)
  • Separate comparison logic into functions (to call if necessary outside the tests)

Used in tests for #749 (PR #864)

Inspired by https://stackoverflow.com/questions/4915462/how-should-i-do-floating-point-comparison

@bragov4ik
separate approx_eq logic into a function
4543ea3
@bragov4ik
add othere kinds of approx equal fns
ef1230e
@bragov4ik
use weak inequalities as tolerance borders should be included
18e77bf
@bragov4ik
use only Fixed and only one comparison call in approx_eq
e555240
@bragov4ik
convert to Fixed in approx_eq_abs macro
048e49a
@bragov4ik
approx_eq errors + some tests
b72e3a2
@bragov4ik
add corresponding macros
ad398a8
@bragov4ik
utilize fixed constants
4da1225
@bragov4ik
test for exact numbers
9bfd5a5
@bragov4ik
abs tolerance match test
582f753
@bragov4ik
reusable and deduplicated test cases
ccc447a
@bragov4ik
similar test for are_approx_eq_abs
d010582
@bragov4ik
similar test for are_approx_eq_rel
89b530c
@bragov4ik
reformat tolerances in comments
2b1efdb
@bragov4ik
more equal numbers tests
fd6c985
@bragov4ik
relative tolerance test
037c231
@bragov4ik
fix comments/values in tests
14a0e90
@bragov4ik
fix tests
6a57244
@bragov4ik
test both matches & no matches
eb91620
@bragov4ik
allow trailing comma in macros
c800088
@bragov4ik
test and fix macros
aed5e4a
@bragov4ik
fix tests and remove dbgs
9721a12
@bragov4ik bragov4ik added enhancement New feature or request CouldHave Nice to have in the release, small impact on the release if not included labels Jan 16, 2024
@bragov4ik bragov4ik added this to the 4.1.0 - ALT milestone Jan 16, 2024
@bragov4ik bragov4ik self-assigned this Jan 16, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 16, 2024
View reviewed changes
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
@bragov4ik bragov4ik linked an issue Jan 16, 2024 that may be closed by this pull request
QA Tools: initialize liquidity source #749
Closed
Alexey-N-Chernyshov
Alexey-N-Chernyshov reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@Alexey-N-Chernyshov Alexey-N-Chernyshov left a comment

Choose a reason for hiding this comment

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

My concern is usage of float-point approximation for fixed-point arithmetics, discussed it with @bragov4ik. Please, reassign me to the PR review in case of changes.

@Alexey-N-Chernyshov
fix(kensetsu): replace assert_approx_eq with assert_eq in xst pallet …
5ccf23a 
…tests (#880)

* fix(kensetsu): replace assert_approx_eq with assert_eq in xst pallet tests
@bragov4ik
add notes about approx_eq usages
99adf27
Alexey-N-Chernyshov
Alexey-N-Chernyshov approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@Alexey-N-Chernyshov Alexey-N-Chernyshov left a comment

Choose a reason for hiding this comment

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

👍

common/src/test_utils.rs Show resolved Hide resolved
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 14, 2024
View reviewed changes
common/src/test_utils.rs Fixed Show fixed Hide fixed
common/src/test_utils.rs Fixed Show fixed Hide fixed
@bragov4ik bragov4ik merged commit e48dea5 into develop Mar 5, 2024
6 checks passed
@bragov4ik bragov4ik deleted the 749-approx-eq branch March 5, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alexey-N-Chernyshov Alexey-N-Chernyshov Alexey-N-Chernyshov approved these changes

@wer1st wer1st wer1st approved these changes

@vovac12 vovac12 Awaiting requested review from vovac12

@N1ghtStorm N1ghtStorm Awaiting requested review from N1ghtStorm

Assignees

@bragov4ik bragov4ik

Labels
CouldHave Nice to have in the release, small impact on the release if not included enhancement New feature or request
Projects
None yet
Milestone
4.0.0 - ALT
Development

Successfully merging this pull request may close these issues.

QA Tools: initialize liquidity source
3 participants
@bragov4ik @Alexey-N-Chernyshov @wer1st