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

speedup function coordinate_to_string #106

Merged
merged 4 commits into from
Mar 11, 2021
Merged

speedup function coordinate_to_string #106

merged 4 commits into from
Mar 11, 2021

Conversation

sdementen
Copy link
Contributor

as the function is used a lot when saving a Workbook, I have simplified it by using a lookup logic (mapping of column letters to column coordinates and vice versa) => 3x faster
I have also updated the function string_to_coordinate in the same spirit (avoid modulo calculation and use lookup)

…to_coordinate to same logic (precalculating mapping between column letters and column coordinate)
Copy link
Collaborator

@kevmo314 kevmo314 left a comment

Choose a reason for hiding this comment

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

Thanks! Mostly looks good, just a question about populating the lookup table.

pyexcelerate/Range.py Outdated Show resolved Hide resolved
pyexcelerate/Range.py Outdated Show resolved Hide resolved
pyexcelerate/Range.py Outdated Show resolved Hide resolved
@kevmo314
Copy link
Collaborator

Awesome, thanks again for the change!

@kevmo314 kevmo314 merged commit 85e42c9 into kz26:dev Mar 11, 2021
@sdementen
Copy link
Contributor Author

I wonder if the two definitions A and Z in Range are still needed or if they were only used for the functions that have been changed

class Range(object):
    A = ord("A")
    Z = ord("Z")

@kevmo314
Copy link
Collaborator

Good catch, I think they can be removed. I don't foresee anyone passing Range.A or Range.Z to any functions meaningfully.

@sdementen
Copy link
Contributor Author

Good catch, I think they can be removed. I don't foresee anyone passing Range.A or Range.Z to any functions meaningfully.

Should I push again the branch with the changes? Or do you take this change in another PR?

@kevmo314
Copy link
Collaborator

Already submitted the change :)

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.

3 participants