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

Fix(load) : with "auto" chunks and type specified #181 #182

Closed
wants to merge 44 commits into from

Conversation

nkarasiak
Copy link

@nkarasiak nkarasiak commented Dec 19, 2024

Fixes issue #181 #184 and #183

@nkarasiak nkarasiak mentioned this pull request Dec 19, 2024
@nkarasiak nkarasiak force-pushed the fix/autochunks_dtype branch from aba9ee6 to 0aec7b7 Compare December 19, 2024 14:30
@nkarasiak
Copy link
Author

Hy there.
I fix an issue with chunks "auto" with dtypes and fix cicd issues too (mypy and wheels error).

Can someone make a feedback ? Thanks for your work 🤗
@Kirill888 @alexgleith @robbibt

@Kirill888
Copy link
Member

@nkarasiak thank you for the contribution, but I have to ask you to undo all the formatting changes please, and also rebase your history into a cleaner set of commits, 21 commits for fixing 3 problems is too many commits. Ideally I'd like to see only changes related to the fix present in the PR. This repo uses black for formatting, not ruff (ruff didn't exist when this was started). Please also remove version bump commit. In general please limit amount of change, especially various "cleanup" changes as they make it harder to understand what the real fix is. For example, I see you made various mypy fixes for dtype annotations, I can't tell if these were needed to fix CI/CD or just there as general improvement.

Right now we are in process of splitting odc/loader/ into a separate package, so it's really important to minimize non-essential changes to the code.

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.54%. Comparing base (d7cdd5c) to head (2fd95ac).
Report is 96 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #182      +/-   ##
===========================================
+ Coverage    87.58%   91.54%   +3.95%     
===========================================
  Files           23       24       +1     
  Lines         1982     2850     +868     
===========================================
+ Hits          1736     2609     +873     
+ Misses         246      241       -5     

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

@nkarasiak
Copy link
Author

Hello @Kirill888,

Thank you for the feedback and for the explanation on how to improve my contribution. That seems really obvious when you explain it ! I'll do that in two weeks.

@nkarasiak
Copy link
Author

I close this pull request, too much drafts.

@nkarasiak nkarasiak closed this Dec 30, 2024
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