Skip to content

Comments

fix int-str conversion in Python 3.11#949

Merged
rocky merged 12 commits intomasterfrom
int-str-conversion-limit
Dec 18, 2023
Merged

fix int-str conversion in Python 3.11#949
rocky merged 12 commits intomasterfrom
int-str-conversion-limit

Conversation

@mmatera
Copy link
Contributor

@mmatera mmatera commented Dec 15, 2023

These are my 2 cents to solve the issue presented in #941

<url>:Python 3.11:https://docs.python.org/3.11/library/stdtypes.html#int-max-str-digits</url>
<dl>
<dt>'$MaxLengthIntStringConversion'
<dd>A system constant that fixes the largest size of the String resulting from converting
Copy link
Member

Choose a reason for hiding this comment

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

String -> 'String' here and below. Integer ->'Integer'
"converting" -> "converting " so there is no hard line break.

Yes, our docstring formatting is weird. I hope to live to see the day when we can ditch it.


class MaxLengthIntStringConversion(Predefined):
"""
<url>:Python 3.11:https://docs.python.org/3.11/library/stdtypes.html#int-max-str-digits</url>
Copy link
Member

Choose a reason for hiding this comment

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

Python 3.11 -> Python 3.11 Integer string conversion length limitation

an Integer into a String.
</dl>

>> originalvalue = $MaxLengthIntStringConversion
Copy link
Member

Choose a reason for hiding this comment

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

The example is good, but I think there should be comments explain better what is going on. In particular, that internally things are okay, but when we get down to displaying the results or formatting, then we have a problem in converting to the output string because it may take a long time.

In a PR to get merged into this, I will work on some suggested expanded verbiage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to improve the description, but I do not know how much we want to go deeper into this.

Copy link
Member

@rocky rocky Dec 18, 2023

Choose a reason for hiding this comment

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

One technique is to give something succinct but then use the examples to elucidate further. Examples though need some additional commentary and motivation was to why there are used, and what to look for, rather than be pure code.

value_str = f"{msd} <<{num_digits - len(lsd)-len(msd)}>> {lsd}"
return String(value_str)


Copy link
Member

@rocky rocky Dec 16, 2023

Choose a reason for hiding this comment

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

We should write a pytest for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just used the WL convention used when the same expression is formatted with Short

Copy link
Member

Choose a reason for hiding this comment

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

Ok - we'll go with this kind of elision then. But we haven't tested it. And this kind of thing is not clear in the docstring either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I put a bunch of pytests. Also, I inaugurated the /test/eval/ directory.

@rocky
Copy link
Member

rocky commented Dec 16, 2023

These are my 2 cents to solve the issue presented in #941

I'll see your 2 cents and double it 4 cents! - Sold? @kejcao Would you please try this and see if this solves your concerns?

@mmatera mmatera force-pushed the int-str-conversion-limit branch from f242297 to 9ec00f5 Compare December 18, 2023 21:46
@mmatera mmatera marked this pull request as ready for review December 18, 2023 21:54
@rocky
Copy link
Member

rocky commented Dec 18, 2023

Whew! We've beat this one to death (for the time being). In the back of my mind is to mention the environment variable DEFAULT_MAX_STR_DIGITS which is set to 7000, but I'll leave that for some other time.

Merge and iterate.

@rocky rocky merged commit 06a2f26 into master Dec 18, 2023
@rocky rocky deleted the int-str-conversion-limit branch December 18, 2023 23:18
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.

2 participants