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
Allow different chunk size #687
Conversation
def _check_copy_to_frontend_kwargs(self, run_id, target, target_frontend_id, rechunk, rechunk_to_mb) -> None: | ||
"""Simple kwargs checks for copy_to_frontend""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must admit I only added this bit because codefactor complained
if rechunk and md["chunk_target_size_mb"] != rechunk_to_mb: | ||
self.log.info(f'Changing chunk-size: {md["chunk_target_size_mb"]} -> {rechunk_to_mb}.') | ||
md.update({'chunk_target_size_mb': rechunk_to_mb}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually the only real change, the rest is cosmetic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a typo in a type annotation, see inline comment. dont think that affects anything but probably should fix before merging.
strax/context.py
Outdated
def copy_to_frontend(self, | ||
run_id: str, | ||
target: str, | ||
target_frontend_id: ty.Optional[int] = None, | ||
target_compressor: ty.Optional[str] = None, | ||
rechunk: bool = False): | ||
rechunk: False = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a typo or some weird syntax I'm not familiar with? shouldn't it still be a bool type hint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely a typo. thanks for spotting!
Allow different chunk target size from original