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

allows naming - move improve performance file #54

Merged
merged 13 commits into from
Feb 26, 2024
Merged

Conversation

svittoz
Copy link
Collaborator

@svittoz svittoz commented Dec 20, 2023

Description

In improve_performance function :

  • Allows app_name parameter so that user can distinguish two kernels from the same project.
  • Improve_performance function definition moved in io folder.

Checklist

  • If this PR is a bug fix, the bug is documented in the test suite.
  • Changes were documented in the changelog (pending section).
  • If necessary, changes were made to the documentation (eg new pipeline).

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4e29cbd) 83.74% compared to head (74a4576) 83.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #54      +/-   ##
==========================================
- Coverage   83.74%   83.22%   -0.53%     
==========================================
  Files          82       83       +1     
  Lines        2492     2492              
==========================================
- Hits         2087     2074      -13     
- Misses        405      418      +13     

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

@TheooJ
Copy link
Contributor

TheooJ commented Jan 17, 2024

Hi Simon !

  • "Allows app_name parameter so that user can distinguish two kernels from the same project." -> this is really useful !
  • "Lazy operations does not seem to work" which operation are you referring to ? Note that some ks operations are rewritten, making them eager:
    def cache(cls, df, backend=None):

Also, I think improve_performance should be accessible from eds_scikit, but shouldn't be written in __init__.py. Writing functions there makes it hard to follow the codebase. I suggest writing it elsewhere and making it accessible by setting from path.to.file import improve_performance in the __init__.py

eds_scikit/__init__.py Outdated Show resolved Hide resolved
@svittoz
Copy link
Collaborator Author

svittoz commented Jan 30, 2024

Hi Simon !

  • "Allows app_name parameter so that user can distinguish two kernels from the same project." -> this is really useful !
  • "Lazy operations does not seem to work" which operation are you referring to ? Note that some ks operations are rewritten, making them eager:
    def cache(cls, df, backend=None):

Also, I think improve_performance should be accessible from eds_scikit, but shouldn't be written in __init__.py. Writing functions there makes it hard to follow the codebase. I suggest writing it elsewhere and making it accessible by setting from path.to.file import improve_performance in the __init__.py

I moved improve_performance into io folder and import it in the init as you suggested.

Copy link
Contributor

@TheooJ TheooJ left a comment

Choose a reason for hiding this comment

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

Added some comments, let me know what you think :)

eds_scikit/__init__.py Outdated Show resolved Hide resolved
eds_scikit/__init__.py Outdated Show resolved Hide resolved
eds_scikit/__init__.py Show resolved Hide resolved
Copy link
Contributor

@TheooJ TheooJ left a comment

Choose a reason for hiding this comment

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

Looks good to me !
Last comment that's a bit nitpick : should we rename the new improve_performance.py file ?

Copy link
Collaborator Author

@svittoz svittoz left a comment

Choose a reason for hiding this comment

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

Moving improve_performance out of init and adding app_name parameter.

Copy link
Collaborator Author

@svittoz svittoz left a comment

Choose a reason for hiding this comment

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

Adding app_name parameter and moving improve_performance out of init.py

Copy link
Collaborator Author

@svittoz svittoz left a comment

Choose a reason for hiding this comment

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

Addin app_name parameter and moving improve_performance.

Copy link
Collaborator

@Thomzoy Thomzoy left a comment

Choose a reason for hiding this comment

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

Ok no problem !

@svittoz svittoz changed the title warns new config values - allows naming allows naming - move improve performance file Feb 26, 2024
@svittoz svittoz merged commit 553677e into main Feb 26, 2024
5 of 6 checks passed
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