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

[Breaking] Split utils into sub-modules, move typing from utils to top-level (pmv.typing) #248

Merged
merged 38 commits into from
Nov 20, 2024

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented Nov 19, 2024

Feel free to move things around.

  • Re-import everything from __init__ to avoid breaking
  • Split tests as well
  • Update the module docstring to include list of available func/...
  • Sort functions alphabetically
  • For test utils, hide from public interface, prefer pmv.utils.testing

@DanielYang59 DanielYang59 added breaking Breaking changes ux User experience api Application programming interface dx Developer experience labels Nov 19, 2024
@DanielYang59 DanielYang59 self-assigned this Nov 19, 2024
@DanielYang59 DanielYang59 changed the title [Breaking] Break utils into sub-modules Break utils into sub-modules Nov 19, 2024
@DanielYang59 DanielYang59 removed the breaking Breaking changes label Nov 19, 2024
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Nov 19, 2024

@janosh Any comment on current categorizing? 😄

from pymatviz.utils import ROOT


TEST_FILES: str = f"{ROOT}/tests/files"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might seem a bit overkill, but I kind of want to reserve this for more test functions like #208

@DanielYang59 DanielYang59 changed the title Break utils into sub-modules Split utils into sub-modules Nov 19, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review November 20, 2024 15:10
@DanielYang59 DanielYang59 marked this pull request as draft November 20, 2024 15:11
@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Nov 20, 2024

Only one task left, I would add a list of available function to the module level docstring of utils.plotting tomorrow

@DanielYang59 DanielYang59 added the breaking Breaking changes label Nov 20, 2024
@DanielYang59 DanielYang59 changed the title Split utils into sub-modules [Breaking] Split utils into sub-modules, move typing from utils to root (pmv.typing) Nov 20, 2024
@janosh janosh marked this pull request as ready for review November 20, 2024 15:15
@janosh janosh merged commit bc2669b into janosh:main Nov 20, 2024
10 checks passed
@DanielYang59
Copy link
Collaborator Author

Thanks! I could sleep with my eyes close now :)

@DanielYang59 DanielYang59 deleted the break-util branch November 20, 2024 15:16
@janosh
Copy link
Owner

janosh commented Nov 20, 2024

thanks so much for your help with this! 🙏

@DanielYang59
Copy link
Collaborator Author

No problem, enjoy your day (and my sleep), thanks for reviewing and all the comments

@DanielYang59
Copy link
Collaborator Author

Looks like the fix for GitHub Pages / build is working now (one extra /), but import performance test is failing for some reason, I would have a closer look tomorrow. Also it seems I forgot to display current time (pymatviz import too slow! hard_threshold=3126.38 ms), only the expected time is shown

@DanielYang59 DanielYang59 changed the title [Breaking] Split utils into sub-modules, move typing from utils to root (pmv.typing) [Breaking] Split utils into sub-modules, move typing from utils to top-level (pmv.typing) Nov 21, 2024
@janosh
Copy link
Owner

janosh commented Nov 27, 2024

i added the expected time. it now prints

FAILED tests/performance_tests/test_import_time.py::test_import_time - Failed: pymatviz import too slow! took 3722 ms, hard_threshold=3126 ms

so at least it's not a massive regression. i'm guessing it's because i added some new modules in pymatviz.classify in #252 and #253 which now need to be parsed so perhaps the slow down is expected? still would be good to find out exactly. could also be i'm importing some module at the top-level which before was only imported at run time. maybe import sklearn.metrics as skm. if that's the case then the test did it's job perfectly :)

@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Nov 28, 2024

i added the expected time. it now prints

Thanks for adding this, I also added one in #241.

i'm guessing it's because i added some new modules in pymatviz.classify in #252 and #253 which now need to be parsed so perhaps the slow down is expected?

I just looked into the commit history and it appears I updated the import time in a6302f2 (I'm completely confused because I cannot remember I re-ran import time benchmark in this PR at all, I originally noticed that update and thought you updated the baseline because of some improvement you made and didn't have time to ask)

I don't think this should be updated by me because I don't expect import time decrease from this PR, and by the time the import time tester was added, the monty fix had already been applied.

In a word, I'm absolutely confused why I updated the import time baseline and where that data came from (as this cannot be "accidentally" updated because the import time test would print a copiable dict instead of overwriting in place), perhaps let's revert it?

@janosh
Copy link
Owner

janosh commented Nov 28, 2024

thanks for checking! i reverted to the old timings in bc1bc29

@DanielYang59
Copy link
Collaborator Author

DanielYang59 commented Nov 28, 2024

Thanks for the revert, though I'm still super confused how this happened and where the data came from ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Application programming interface breaking Breaking changes dx Developer experience ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants