Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #529 +/- ##
==========================================
- Coverage 88.85% 88.83% -0.03%
==========================================
Files 113 113
Lines 13090 13152 +62
Branches 2421 2433 +12
==========================================
+ Hits 11631 11683 +52
- Misses 914 922 +8
- Partials 545 547 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables NCEM EMD reading to support true lazy-loading by allowing _read_dataset() to return an h5py-backed object instead of eagerly materializing dataset[:].
Changes:
- Add a
lazyoption to_read_dataset()and propagate it through the NCEM data reading paths. - Update dask wrapping code paths to pass
lazy=self.lazy. - Add an upcoming_changes entry documenting the fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| upcoming_changes/526.bugfix.rst | Documents the NCEM lazy-loading behavior change. |
| rsciio/emd/_emd_ncem.py | Adds lazy support to _read_dataset() and updates callers (numpy/dask paths). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data_list = [ | ||
| da.from_array(*self._read_dataset(d)) for d in dataset_list | ||
| da.from_array(*self._read_dataset(d, lazy=self.lazy)) | ||
| ] |
|
|
||
| data_list = [ | ||
| da.from_array(*self._read_dataset(d)) for d in dataset_list | ||
| da.from_array(*self._read_dataset(d, lazy=self.lazy)) |
| import dask.array as da | ||
|
|
||
| data = da.from_array(*self._read_dataset(d)) | ||
| data = da.from_array(*self._read_dataset(d, lazy=self.lazy)) |
|
|
||
| data_list = [ | ||
| da.from_array(*self._read_dataset(d)) for d in dataset_list | ||
| da.from_array(*self._read_dataset(d, lazy=self.lazy)) |
| import dask.array as da | ||
|
|
||
| data = da.from_array(*self._read_dataset(d)) | ||
| data = da.from_array(*self._read_dataset(d, lazy=self.lazy)) |
| @@ -0,0 +1 @@ | |||
| Modified _emd_ncem.py to correctly handle lazy loading of ncem data by avoiding eager dataset reads. No newline at end of file | |||
| data_list = [ | ||
| da.from_array(*self._read_dataset(d)) for d in dataset_list | ||
| da.from_array(*self._read_dataset(d, lazy=self.lazy)) | ||
| ] |
| if len(dataset_list) > 1: | ||
| # Squeeze the data only when | ||
| if self.lazy: | ||
| import dask.array as da | ||
|
|
||
| data_list = [ | ||
| da.from_array(*self._read_dataset(d)) for d in dataset_list | ||
| da.from_array(*self._read_dataset(d, lazy=self.lazy)) | ||
| ] | ||
| if transpose_required: | ||
| data_list = [da.transpose(d) for d in data_list] | ||
| data = da.stack(data_list) | ||
| data = da.squeeze(data) |
|
Most of the co-pilot review comments sound sensible and would need to be addressed. |
For py4DSTEM files, parse the calibration information under [root]/metadatabundle/calibration Updated test files to use a new py4DSTEM file with multiple datasets
|
Sorry for being late. In the new commitment, I have:
All tests passed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
upcoming_changes/526.bugfix.rst:1
- The news fragment should be written for users (per upcoming_changes/README.rst) rather than referencing internal module names. Consider describing the user-visible behavior change (lazy loading) instead of “Modified _emd_ncem.py”.
Modified _emd_ncem.py to correctly handle lazy loading of ncem data by avoiding eager dataset reads.
rsciio/tests/test_emd_ncem.py:118
lazyis parametrized but not passed tofile_reader, so this test only exercises the eager code path and won’t catch regressions in lazy loading. It also mutatessignalswhile iterating and can raise an unhelpfulUnboundLocalErrorif the 4D signal isn’t found.
def test_py4dstem(lazy):
filename = TEST_DATA_PATH / "py4DSTEM_size2x3x4x5_bf20x30.h5"
signals = file_reader(filename)
assert len(signals) == 2
rsciio/emd/_emd_ncem.py:419
- This block eagerly reads every dataset under
metadatabundle/calibrationinto memory (value[()]) even whenself.lazyis True. If any calibration entries are large arrays, this undermines the memory benefits of lazy loading and can add significant IO during metadata parsing.
calibration = {}
for key, value in calibration_group.items():
if isinstance(value, h5py.Dataset):
calibration[key] = value[()]
om.update({"calibration": calibration})
|
Copilot comments 1 and 2 are addressed. 3 is less of a concern because those strings are short. |
|
@ericpre Sorry it has been a busy summer — is there anything I have to do with this one? |
Description of the change
Modified the _read_dataset() function in _emd_ncem.py to take lazy options, instead of always eagerly reading the dataset by assigning data = dataset[:]. All tests passed.
Progress of the PR
upcoming_changesfolder (seeupcoming_changes/README.rst),docs/readthedocs.org:rosettasciiobuild of this PR (link in github checks)