Skip to content

gh-108303: Move all math files to Lib/test/mathdata/ #109512

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

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Sep 17, 2023

This one is pretty straight-forward. All math data now lives in a specialized place.

lizzydavis695

This comment was marked as spam.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I dislike "data" directories.

It seems like test_math uses 3 data files which are only used by test_math.

test_float uses 2 data files which are only used by test_float.

For me, "data" directories are nice when data files are shared by different tests. It doesn't seem to be the case here.

What do you think of creating test_float/ and test_math/ directories instead?

@sobolevn
Copy link
Member Author

What do you think of creating test_float/ and test_math/ directories instead?

I don't have any strong opinions here, for me mathdata/ seems like a more simple way of organizing tests. But, I can change it to be test_math/ (with possibly test_cmath.py inside) and test_float/

Why do I prefer the first option? Because later some other file can be added there: possibly from test_int.py or test_decimal.py or whatever. Or some test from test_math.py can reuse float test data. This is why I went with the approach in this PR.

@serhiy-storchaka
Copy link
Member

"cmath_testcases.txt" is shared with test_cmath.

@vstinner
Copy link
Member

"cmath_testcases.txt" is shared with test_cmath.

test_cmath uses dark black magic, it uses class inheritance:

class IsCloseTests(test_math.IsCloseTests):

So it doesn't directly reuses "cmath_testcases.txt", but uses it "via" test_math. Oh, that's "surprising".

@vstinner
Copy link
Member

@serhiy-storchaka approved the change, I'm not strongly against Lib/test/mathdata/ approach.

I'm just surprised that 2 test files are added there whereas they are only used by test_float. I don't see the point of "sharing" these two files. Maybe make a test_float/ directory for them?

@serhiy-storchaka: If you're fine with mathdata/ approach,go ahead and merge the PR :-)

Because later some other file can be added there: possibly from test_int.py or test_decimal.py or whatever. Or some test from test_math.py can reuse float test data.

That sounds to be be an hypothetical use case. I would prefer to stick to current test data files usage to decide how to organize them.

@serhiy-storchaka
Copy link
Member

It also imports the path to the test data file and the parsing function from test_math, and runs tests without skipping complex numbers.

@vstinner
Copy link
Member

I can see the point of having mathdata/ for test_math and test_cmath.

What about test_float test files?

@serhiy-storchaka
Copy link
Member

I'm just surprised that 2 test files are added there whereas they are only used by test_float. I don't see the point of "sharing" these two files. Maybe make a test_float/ directory for them?

They could be used in tests for decimal and fractions. I am not even sure that they are not yet used there.

@vstinner
Copy link
Member

They could be used in tests for decimal and fractions.

If tomorrow other tests start using test_float data files, we can re-consider the organization of these data files, no? Are you proposing to make this change right now?

I am not even sure that they are not yet used there.

You can easily test: remove data files, run tests, do they still pass? :-)

The purpose of #108303 is to clarify how test data files are used ;-)

@rhettinger
Copy link
Contributor

To me, these kind of edits (mostly rearrangements) have no value. It is mostly just churn.

@serhiy-storchaka
Copy link
Member

If tomorrow other tests start using test_float data files, we can re-consider the organization of these data files, no?

#109548

@vstinner
Copy link
Member

Oh, @serhiy-storchaka made #109548 which makes mathdata/ directory more relevant :-)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner vstinner merged commit ed587be into python:main Sep 21, 2023
@vstinner vstinner added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 21, 2023
@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Thanks @sobolevn for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @sobolevn and @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ed587be0d0383f2925bf7650e8ccf1bf3adc44f9 3.11

@miss-islington
Copy link
Contributor

Sorry, @sobolevn and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker ed587be0d0383f2925bf7650e8ccf1bf3adc44f9 3.12

vstinner pushed a commit to vstinner/cpython that referenced this pull request Sep 21, 2023
@vstinner
Copy link
Member

Oh, this PR was outdated since PR #109548 got merged! I wrote PR #109686 to fix it.

csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@ZeroIntensity ZeroIntensity removed needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants