Stop the _adapt_batch() from changing the batch in-place#306
Stop the _adapt_batch() from changing the batch in-place#306dfulu merged 3 commits intoopenclimatefix:mainfrom
Conversation
|
Hi @adityasuthar, thanks for doing this. I think this won't fully cover the issue. The batch object is a nested dictionary (max depth of 2) so the line you have in to copy it only creates a shallow copy, and the deeper part of the dict can is still changed. The problem is probably best shown with an example: |
|
Hi @dfulu, Thanks for reviewing the code and pointing out the issue with the To correctly copy both the first-level and nested structures, I updated the approach to:
This ensures all nested elements are fully copied, preventing unwanted modifications. Example with Fix Applied: |
98520df to
96408c9
Compare
dfulu
left a comment
There was a problem hiding this comment.
Hi @adityasuthar, thanks for the work. Looks good!
Pull Request
Description
The _adapt_batch() method changes the batch in place. This is bad practice. We are updating the method so that it returns a new batch without changing the old one.
Fixes #210
How Has This Been Tested?
I tested by running the
python -m pytest testscommand.If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?
Checklist: