Skip to content

Better defaults for chunks parameters #704

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jonbrenas
Copy link
Collaborator

Addresses #629.

This is just a prototype to check that it is what @alimanfoo had in mind. I have not tested that it actually improves performances yet.

If that's the correct idea, I will extend that to other similar functions.

@leehart leehart requested a review from alimanfoo January 24, 2025 11:31
@leehart
Copy link
Collaborator

leehart commented Mar 21, 2025

@alimanfoo

@leehart
Copy link
Collaborator

leehart commented May 22, 2025

I'll try re-running the integration and notebook tests here, to see if it automatically picks up the fix for gcsfs.

Ref: issue #782

@leehart
Copy link
Collaborator

leehart commented May 22, 2025

By the way, did we ever find out whether this was what was in mind? I wonder if there is a way to confirm, e.g. if performance is improved and nothing breaks and it seems to make sense to others, etc.

@leehart leehart removed the request for review from alimanfoo May 22, 2025 11:13
@jonbrenas
Copy link
Collaborator Author

jonbrenas commented May 22, 2025

I am not sure that was exactly what @alimanfoo had in mind. I think there is a piece of chunk optimisation in the PCA part of the cohort analysis which is what he meant to do but I have not checked that it does the same thing as this. I'll move this PR back to draft to force myself to remember to check it.

@jonbrenas jonbrenas marked this pull request as draft May 22, 2025 14:22
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