#66 loosen tolerance for comparing memory consumption #67

jschneidereit · 2024-07-21T00:32:32Z

Not sure how I feel about the actual fix to the test here, order of magnitude for the memory requested is 1 and 10^2 is just 10 which feels better than 5 to me 😛.

Not sure why this M chip is using up one more megabyte but the tests pass on osx-arm64 now!

(fixes #66)

elevans

Hi @jschneidereit. thanks for this PR. Overall I like the changes your suggest. My main critique that I have is that you added a new dependency for assertpy which I'm generally not in favor for. Can you implement your memory consumption relaxation without this new package?

Also please run the black code formatter to resolve the clean code failures.

elevans · 2024-07-22T21:35:53Z

dev-environment.yml

@@ -28,11 +28,11 @@ dependencies:
  - numpy
  - pandas
  # Developer tools
+  - assertpy


Do we need this package for this fix? I appreciate that it cleans up some of the test code for readability, however I would prefer to not add another dependency to scyjava.

@elevans If it were a transitive dependency for the main codebase I'd fully agree, but since it's only a developer dependency... I think I feel less strongly than you do about it. 😅

Can I interest you in the power of readable test failure messages?

This is the original error message:

assert abs(mb_total - mb_initial) < 5, f"{mb_total=} {mb_initial=}" ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ AssertionError: mb_total=56 mb_initial=50

While this is th error message using assertpy:

AssertionError: [total mb should be close to initial] Expected <56> to be close to <50> within tolerance <5>, but was not.

The value becomes more clear when you write tests checking more complex things than number comparisons, take a scroll through their README to see if you recognize any patterns 😎

A more concrete example is near the end of test_arrays.py:

for i in range(len(nums_delta)): for j in range(len(nums_delta[i])): assert nums_delta[i][j] == pdoubles[i][j]

If you used assertpy you'd get a nice error message displaying both lists showing how they differ instead of just "false"

for i in range(len(nums_delta)): assert_that(nums_delta[i]).is_equal_to(pdoubles[i])

Persuasion successful! Curtis is right and perhaps I was too against adding a new dependency. Since this would just be in the development config and not the main code base I have no problems here and in fact I think we could use assertpy in pyimagej and perhaps imglyb!

elevans · 2024-07-22T21:39:04Z

tests/it/java_heap.py

-# Most of that memory should still be free; i.e.,
-# we should not be using more than a few MB yet.
-assert mb_used <= 5, f"{mb_used=}"
+tolerance = pow(10, magnitude(mb_initial))


Add a comment about what tolerance does here.

This logic feels like overkill to me... I would have just changed the 5 to 10 and called it good 😆

😂 you're not wrong, somehow felt better than just "10" - I'll change it to that

I'll wait to add the comment on the above discussion, in test assertion frameworks they generally let you say how close two numbers should be to each other within some magnitude. This is that, but if y'all put the kibosh on assertpy then it would just be < 10 (which I would still rename to tolerance or delta or something as opposed to an inlined number)

We will keep assertpy but I also agree with @ctrueden that this does feel a bit much. I say just set the tolerance to <10.

ctrueden

I kinda like the elegance of the assert_that calls. I would personally suggest to leave off the magnitude reasoning though, in favor of just hardcoding the tolerance to 10.

@elevans I defer to you on both decisions to be made here:

Do we keep the new assertpy dev dependency, or stick with built-in Python asserts?
Do we keep the magnitude reasoning, or just set it to 10 (or 15 or whatever)?

ctrueden · 2024-07-23T02:39:55Z

dev-environment.yml

@@ -28,11 +28,11 @@ dependencies:
  - numpy
  - pandas
  # Developer tools
+  - assertpy


@elevans If it were a transitive dependency for the main codebase I'd fully agree, but since it's only a developer dependency... I think I feel less strongly than you do about it. 😅

ctrueden · 2024-07-23T02:42:03Z

tests/it/java_heap.py

-# Most of that memory should still be free; i.e.,
-# we should not be using more than a few MB yet.
-assert mb_used <= 5, f"{mb_used=}"
+tolerance = pow(10, magnitude(mb_initial))


This logic feels like overkill to me... I would have just changed the 5 to 10 and called it good 😆

elevans · 2024-07-23T20:49:55Z

Do we keep the new assertpy dev dependency, or stick with built-in Python asserts?

I agree that assertpy makes the tests and error outputs easier to read. The make or break for me was making sure we do not add more dependencies to main code base, which as you pointed out this would not. Thus I say we keep assertpy and perhaps apply it to pyimagej and imglyb down the road 🚀!

Do we keep the magnitude reasoning, or just set it to 10 (or 15 or whatever)?

I think the magnitude reasoning is more complicated than what we need it to be. I prefer that the desired tolerance is
just set. @jschneidereit Is there some good reasoning that I'm not thinking about about keeping the magnitude function?

ctrueden · 2024-07-23T21:53:59Z

I took the liberty of pushing commits fixing the linting errors and simplifying the tolerance logic.

But still only three Pythons. Five Pythons is too many Pythons.

codecov · 2024-07-23T22:02:18Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.15%. Comparing base (4ef3781) to head (b414c55).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #67   +/-   ##
=======================================
  Coverage   53.15%   53.15%           
=======================================
  Files          12       12           
  Lines        1285     1285           
=======================================
  Hits          683      683           
  Misses        602      602

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ctrueden · 2024-07-23T22:06:03Z

Thank you @jschneidereit for the very tidy contribution!

jschneidereit added 4 commits July 20, 2024 17:26

chore: move flake8-typing-imports to pip section

da30759

chore: add assertpy for readable test failures

a7a1517

chore: refactor java_heap tests for readable errors

a1e48da

fix: loosen tolerance for comparing memory consumption

56df799

elevans self-assigned this Jul 22, 2024

elevans requested changes Jul 22, 2024

View reviewed changes

ctrueden reviewed Jul 23, 2024

View reviewed changes

Make the linter happy again

fa38c52

ctrueden force-pushed the jschneidereit/66-fix-osx-arm64 branch from 433abae to 88769c4 Compare July 23, 2024 21:53

ctrueden added 2 commits July 23, 2024 16:56

Simplify tolerance reasoning in java_heap test

eec97e8

Test Pythons from 3.8 - 3.12

b414c55

But still only three Pythons. Five Pythons is too many Pythons.

ctrueden force-pushed the jschneidereit/66-fix-osx-arm64 branch from 88769c4 to b414c55 Compare July 23, 2024 21:57

ctrueden merged commit bf5910e into scijava:main Jul 23, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#66 loosen tolerance for comparing memory consumption #67

#66 loosen tolerance for comparing memory consumption #67

jschneidereit commented Jul 21, 2024 •

edited

Loading

elevans left a comment •

edited

Loading

elevans Jul 22, 2024

ctrueden Jul 23, 2024

jschneidereit Jul 23, 2024

elevans Jul 23, 2024 •

edited

Loading

elevans Jul 22, 2024

ctrueden Jul 23, 2024

jschneidereit Jul 23, 2024

jschneidereit Jul 23, 2024

elevans Jul 23, 2024

ctrueden left a comment

ctrueden Jul 23, 2024

ctrueden Jul 23, 2024

elevans commented Jul 23, 2024 •

edited

Loading

ctrueden commented Jul 23, 2024

codecov bot commented Jul 23, 2024

ctrueden commented Jul 23, 2024

#66 loosen tolerance for comparing memory consumption #67

#66 loosen tolerance for comparing memory consumption #67

Conversation

jschneidereit commented Jul 21, 2024 • edited Loading

elevans left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevans Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrueden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevans commented Jul 23, 2024 • edited Loading

ctrueden commented Jul 23, 2024

codecov bot commented Jul 23, 2024

Codecov Report

ctrueden commented Jul 23, 2024

jschneidereit commented Jul 21, 2024 •

edited

Loading

elevans left a comment •

edited

Loading

elevans Jul 23, 2024 •

edited

Loading

elevans commented Jul 23, 2024 •

edited

Loading