-
Notifications
You must be signed in to change notification settings - Fork 2
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 chunking issues in sum_AMEL and reduce_damages #83
Conversation
Codecov Report
@@ Coverage Diff @@
## dscim-v0.4.0 #83 +/- ##
================================================
+ Coverage 67.99% 68.21% +0.21%
================================================
Files 17 17
Lines 1859 1878 +19
================================================
+ Hits 1264 1281 +17
- Misses 595 597 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
save_path str | ||
Path to save concatenated file in .zarr format | ||
""" | ||
paths = glob.glob(f"{damage_dir}/{basename}*") |
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 usually prefer to explicitly create a list of filenames to open, in case there's extra data files or anything like that. Maybe that's handled in a data check later?
|
||
for v in list(data.coords.keys()): | ||
if data.coords[v].dtype == object: | ||
data.coords[v] = data.coords[v].astype("unicode") |
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.
might as well handle this in a unit test to add the coverage and avoid the warning
data.coords[v] = data.coords[v].astype("unicode") | ||
for v in list(data.variables.keys()): | ||
if data[v].dtype == object: | ||
data[v] = data[v].astype("unicode") |
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.
same as above comment
"ssp": 1, | ||
} | ||
else: | ||
chunkies = { |
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.
please add to unit tests
.rename(var) | ||
.chunk( | ||
{ | ||
"batch": 15, |
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.
seeing this dictionary of chunks repeated many times confirms that we should generalize at least a little bit - perhaps define a global chunkies and eventually put into a config. This can be done in a later PR.
We decided to add the test coverage and generalizing of chunk sizes to later PRs. |
No description provided.