Repository navigation
Conversation
….7 results as golden files The EIT tests had only a Draeger file, so the Sentec and Timpel readers were never checked. Both new files come from the eitprocessing test data on Zenodo (CC BY 4.0). The Sentec file is the whole recording, and the Timpel file keeps 60 s to 120 s of its recording. NOTICE.md gives the credit, and Git LFS stores the .zri files. The EIT code from eitprocessing moves into m3resp, and the new code must give the same numbers. scripts/make_eit_golden_files.py runs eitprocessing on the three EIT test files and saves every result in .npz files, with a manifest of the version, commit, settings and file checksums. The new test compares eitprocessing itself and the m3resp EIT steps with these files. Git LFS stores the golden files.
The EIT code moves from eitprocessing into m3resp, and file reading is the first part. The readers in m3resp.eit.io are copied from eitprocessing 1.8.7. Each returns an EITFileData with the images and waveforms as Signal, the device marks as EventData and the Timpel device breaths as BreathEvent. eit.load uses them through read_eit_file. The steps after eit.load still read an eitprocessing Sequence, so file_data_to_sequence builds the same Sequence from the reader result. Four reading cases stopped with an error in eitprocessing. They give a result or a clear error here: Sentec with first_frame and max_frames together, Timpel with zero or one frame left, Timpel device breaths when fewer than two minima remain, and the vendor "simulated". All other results equal eitprocessing. The tests compare the readers with the golden files and with eitprocessing, and the golden test also checks the ventilator waveforms read from the EIT file.
Sentec and Timpel images have NaN for the pixels outside the chest, so two copies of the same images were "not equal" with ==. TimeSeries, Signal and ParameterResult compare float arrays with equal_nan, so a NaN at the same place in both values counts as equal. A NaN where the other value has a number still makes them different, and text values compare as before.
…sp code The file readers give m3resp data, but the EIT steps still passed eitprocessing objects, and the impedance sums and slicing still ran in eitprocessing. This is the next part of the EIT code that moves into m3resp. EITRecording holds the native data next to the Sequence: pixel_impedance, signals, events, intervals and raw_global_impedance. join_eit_files joins files recorded one after the other. The EIT steps give and read Signals for the images and impedance waveforms, and the adapter converts them to EITData or ContinuousData before each eitprocessing call. global_impedance and regional_impedance in m3resp.eit.processing sum the pixels, and PixelMask.apply masks an array, a Signal or a PixelMap. session.slice_eit and eit.slice_recording cut the native images, waveforms, device marks and breaths with slice_file_data, then rebuild the Sequence. eit.slice_signal accepts start_inclusive and end_inclusive in time mode. The ventilator reader takes its waveforms from the native recording. The new tests check that the sums, masked pixels and slices match eitprocessing 1.8.7. NOTICE.md lists the new files copied from eitprocessing.
The docs still said that m3resp loads and slices EIT data through eitprocessing. The concept pages, the migration guide and the API reference describe the fields of the native recording, the artifact types of the EIT Signals, PixelMask.apply, the slice rules for device marks and breaths, and the new functions. The changelog lists these changes and the NaN fix.
…esp code Rate detection and breath detection still ran in eitprocessing. detect_rates and find_breaths in m3resp.eit.processing are copied from eitprocessing 1.8.7 and give the same numbers, with the same settings and defaults. The EIT steps and session.preprocess_eit() use them, and the breaths come out as BreathEvent.
The MDN and Butterworth filters of the EIT steps still ran in eitprocessing. mdn_filter in m3resp.eit.processing is copied from eitprocessing 1.8.7 and gives the same numbers, and eit.mdn_filter has three new settings: noise limit, notch distance and filter order. eit.butterworth_filter and session.preprocess_eit() use m3resp.processing.filters.butterworth_filter, with order 1 to 10. Both filters give a Signal, and the run summary shows "mdn" for the MDN step. The tests compare both filters with the golden files and with eitprocessing on the three EIT test files.
…of another modality tidal_impedance_variation and end_expiratory_lung_impedance in m3resp.eit.processing are copied from eitprocessing 1.8.7 and give the same numbers. They take BreathEvents and give an IntervalData (#113). eit.continuous_tiv and eit.eeli have a new input breaths for EIT, EMG or ventilator breaths, which they move to the EIT clock. ventilator.normalize_breaths gives its breaths as ventilator_breath_events. The outputs are renamed to continuous_tiv_result and eeli_result, and eit.continuous_tiv has the new setting tiv_method. plot_eit_processing_summary also stopped with an AttributeError after a workflow run, because the global impedance is a Signal there. This is fixed. The tests compare both functions with the golden files and with eitprocessing, and run the workflow on ventilator and EMG breaths.
… with m3resp code The pixel steps still ran in eitprocessing. find_pixel_breaths and pixel_tidal_impedance_variation are copied from eitprocessing 1.8.7 and give the same numbers. pixel_end_expiratory_lung_impedance and the step eit.pixel_eeli are new (#114). The new type PixelBreaths holds the breath times of each pixel (#136). eit.pixel_tiv has the new setting tiv_method and the optional inputs breaths and pixel_breaths. The output pixel_tiv and the input eit_sequence are removed. The tests compare the results with the golden files and with eitprocessing.
The steps eit.roi_tiv_lungspace, eit.roi_amplitude_lungspace, eit.roi_watershed and eit.roi_filter_by_size used to call eitprocessing and returned eitprocessing masks. They now run the new m3resp.eit.roi functions, copied from eitprocessing 1.8.7, and return m3resp PixelMask objects. With the default settings, the masks of the three test files match those from eitprocessing. The three lung space steps take an optional breaths input. The new step eit.geometric_mask returns a half, a layer or a quadrant of the EIT image. PixelMap has a new field kind and the methods of the eitprocessing PixelMap. The old outputs such as watershed_lungspace_mask are removed. A spec now binds the size filter to watershed_lungspace_result. The watershed method needs skimage, which is a main dependency.
…esp code The GUI draws the EIT results in the browser and needs the same colours as the figures. plot_pixel_map draws a PixelMap or a PixelMask with the colours and the colour bar of its kind, the same as imshow of eitprocessing 1.8.7. The colours are stored in pixel_map_colors.json (made by scripts/make_pixel_map_colors.py), so the GUI reads them without Matplotlib. plot_data gives the maps, masks, EIT images, lung space captures and rate spectra as numbers and lists that json.dumps writes as they are. minute_image gives the mean pixel TIV over the breaths inside a time window, for the Prepare page of the GUI. pixel_tiv.png of save_eit_figures is drawn with plot_pixel_map, so it is also written without eitprocessing. The picture is the same as before, pixel for pixel.
psomhorst
left a comment
There was a problem hiding this comment.
This is the first part of my review of this PR.
I reviewed the first two commits. The first was about creating golden files, which I only skimmed through.
The translation of the EIT loaders is rather straightforward, with some caveats. The general code and structure makes sense and is readable.
I am not quite sure about the module nesting. I would expect m3resp.io.eit... and m3resp.processing.eit..., but found m3resp.eit.io... and m3resp.eit.processing.... I think we should keep the option open to make M3RESP as general purpose as possible. Organizing modules into three silos is not conductive to that.
There is a single bug that I recently fixed in eitprocessing (PR under review), but that is present in this code.
There are some design choices in eitprocessing that maybe should be reconsidered.
There are some omissions in data loading, that should be fixed at some point. I don't think that's crucial for stage 3, however. I created #150, #151 and #152 to address these.
| ("draeger", "global_impedance_(raw)"): "impedance", | ||
| ("timpel", "global_impedance_(raw)"): "global_impedance", |
There was a problem hiding this comment.
Why do these differ? They are created in the same way, i.e., by summing the pixel impedance (EITData.calculate_global_impedance())
There was a problem hiding this comment.
The difference comes from eitprocessing 1.8.7. Its Draeger loader gives the raw global impedance the category "impedance", and its Timpel loader gives it "global_impedance". This table copies those values so that the result equals load_eit_data field by field.
All code in adapters/eitprocessing_adapter/ is temporary. It only turns the output of the m3resp readers into an eitprocessing Sequence. The EIT reading and all EIT calculations now run in m3resp's own code, so this whole folder, including this table, will be removed by the end of stage 3.
The m3resp readers (eit/io/draeger.py and eit/io/timpel.py) have the same difference. #156 removes this signal from the readers, so the difference goes there too.
| # The value is kept as given: the default is the whole number 50, as in | ||
| # eitprocessing. | ||
| frequency = sample_frequency |
There was a problem hiding this comment.
This is a useless assignment.
Also, this reads as a AI-generated comment after some consideration of where this number comes from. Without more context, the comment does not make sense.
There was a problem hiding this comment.
Agreed, I removed the assignment and the comment. The reader now sets the sample frequency once, as a float, and uses it for the images and all waveforms.
The comment came from matching eitprocessing exactly: there the images get 50.0 and the waveforms get the whole number 50. The CHANGELOG lists this as a difference.
psomhorst
left a comment
There was a problem hiding this comment.
These are comments up to fc20aef.
You have me somewhat confused again.
Am I correct in concluding that session has not implemented the native loaders yet? M3Session uses EITProcessingAdapter which uses eitprocessing for loading.
Other than that, I don't understand why loading functionality is split between the modules modalities/eit.py and eit/io/.
modalities/eit.py is quite confusing and messy. I thought the EITProcessingAdapter was temporary, but it is still used. Is this still temporary for regression testing purposes?
I am also very confused about what final object contains EIT data that is processed. That should be M3Session.signals, right? I don't see when/where EIT data is stored there, other than in workflows. EMG/ventilator data gets put in M3Session.signals in M3Session's preprocess functions.
EITFileData seems to be an intermediate object used to hand off data between loading and storing in EITRecording, but can also be sliced? EITRecording has mostly recorded data, but also some calculated data, so this is also used somewhere for processing? Or is this purely for visualization of data before doing any processing?
(sorry if this is explained in the documentation; docs won't build on my side; it hangs on generating gallery for tutorials... [ 50%] 10_eit_only.py)
| def _arrays_equal(first: np.ndarray, second: np.ndarray) -> bool: | ||
| """Compare two arrays; NaN values at the same places count as equal.""" | ||
|
|
||
| both_float = first.dtype.kind in "fc" and second.dtype.kind in "fc" | ||
| return bool(np.array_equal(first, second, equal_nan=both_float)) |
There was a problem hiding this comment.
Why is a separate function necessary? Would using np.array_equal(..., equal_nan=True) not suffice everywhere? equal_nan=True works always, even if either/both of the arrays are not float arrays.
There was a problem hiding this comment.
equal_nan=True works for whole and decimal numbers, but it raises a TypeError for text and object arrays (ufunc 'isnan' not supported for the input types, numpy 2.5.1). numpy looks for NaN with isnan, which works only on numbers.
The helper turns on equal_nan only when both arrays hold decimal numbers, so text values compare as before.
test_text_values_compare in tests/test_data_model_runtime.py covers this case. ParameterResult == uses the same helper.
| GAP_TOLERANCE_FRAMES = 0.5 | ||
|
|
||
|
|
||
| def join_eit_files(files: Sequence[EITFileData], paths: Sequence[str]) -> EITFileData: |
There was a problem hiding this comment.
This function makes sense.
Two things.
First, the paths are in EITFileData, so they don't have to be provided separately, right?
Second, the naming is slightly confusing. Something join_eit_file_data() would be more clear. Currently, it seems this is a helper function to merge files (on disk).
There was a problem hiding this comment.
Agreed on both. The function is now join_eit_file_data(files). It takes the file names for its messages from the EITFileData items, so the separate list of paths is gone. check_file_order lost the same argument.
|
|
||
| Returns: | ||
| EITFileData: All frames, waveforms, marks and breaths in one | ||
| recording. ``path`` is the path of the first file. |
There was a problem hiding this comment.
I am not sure I agree with this. It removes easily accessible provenance information about where the data comes from, compared to a list of paths.
There was a problem hiding this comment.
Agreed. EITFileData.path is now paths, the files in recording order, like EITRecording.paths. A join keeps the paths of all files, and cutting a recording with slice_eit keeps them too.
There was a problem hiding this comment.
I think I don't agree with the workflow this file suggests.
The workflow should generally be:
- other preprocessing steps
- create mask
- apply mask to pixel data
- sum to get regional / functional data
- use masked pixel data for other preprocessing steps
If you mask pixel data, and then put it in the function global_impedance(), you don't get global impedance, but regional/functional impedance. Unless it is possible to track whether you've masked the pixel data and use the non-masked version instead to calculate global impedance.
For functional lungspace the workflow should also probably be: mask pixel data, then sum or do other things with it. However, the function could still be useful as a shorthand.
There was a problem hiding this comment.
Agreed, this was possible without any warning.
PixelMask.apply now lists the name of the mask under metadata["masks"] of the masked images, and a second mask is added to the list. global_impedance raises an error for masked images and points to regional_impedance.
The module text now describes the order you give: filter, make the mask, apply it, then use the masked images. regional_impedance and functional_impedance stay as the shorthand for applying a mask and summing in one call.
| name: str = "regional_impedance", | ||
| channel: str = "regional_impedance", |
There was a problem hiding this comment.
I think it could make sense to derive the name from the name of the mask. If no name is supplied, the name {mask.name}_impedance is maybe a safe bet. Or did we decide earlier to never implicitly name data?
There was a problem hiding this comment.
Agreed. We have no rule against it, and PixelMap.threshold and filter_by_size already make the name from their input.
With no name given, the result is now {mask.name}_impedance.
| start (float): Start of the window, in seconds. | ||
| end (float): End of the window, in seconds. |
There was a problem hiding this comment.
There is a risk of confusing start/end indices with start/end times. Functions in this module accept either, but it is not clear from the function or attribute name which. In other places you do use start_index and start_seconds (I think it makes more sense to use the quantity name instead of the unit).
| start (float): Start of the window, in seconds. | |
| end (float): End of the window, in seconds. | |
| start_time (float): Start of the window, in seconds. | |
| end_time (float): End of the window, in seconds. |
For functions that use indices instead: start_index and end_index.
NB: I noticed we didn't do this consistently in eitprocessing either. Sorry!
There was a problem hiding this comment.
Agreed.
The functions that take times now use start_time and end_time: slice_events, interval_indices and slice_intervals here, and slice_by_time and time_slice_indices in m3resp.processing.slicing.
The functions that take sample or frame numbers use start_index and end_index: slice_file_data and slice_by_index. The step eit.slice_signal keeps start and end, because its mode setting says whether they are times or indices.
The session slice methods and the slice_recording steps of EIT, EMG and ventilator still use start_seconds and end_seconds. Those are setting names in the workflow files and the GUI, so I would rename them together with #165.
| "Slicing by time needs a sorted, finite, one-dimensional time axis." | ||
| ) | ||
| if any(value is not None and not np.isfinite(value) for value in (start, end)): | ||
| raise ValueError("Slicing bounds must be finite times in seconds.") |
There was a problem hiding this comment.
| raise ValueError("Slicing bounds must be finite times in seconds.") | |
| raise ValueError("Slicing bounds must be finite times in seconds or None.") |
|
|
||
| The EIT images and impedance waveforms that the EIT steps give and read are `Signal`s: `raw_eit`, `raw_global_impedance`, `filtered_eit`, `global_impedance`, `regional_impedance` and `functional_impedance`. Before, these were eitprocessing `EITData` and `ContinuousData` objects. The artifact types keep their names: `eit_pixel_signal` is a `Signal` with EIT images (frames, rows, columns) and `eit_impedance_waveform` a `Signal` with one impedance value per frame. They are no longer marked as compatibility types. The outputs with the same Signal under a second name (`raw_pixel_impedance_signal`, `filtered_eit_signal`, ...) stay and have the same artifact type. A step input of type `signal` accepts both EIT types; an input of type `eit_pixel_signal` refuses a waveform before the workflow runs. A step that calls eitprocessing turns its `Signal` inputs into eitprocessing objects first; `m3resp.adapters.eitprocessing_adapter` has the conversions (`signal_to_eit_data`, `signal_to_continuous_data` and back). | ||
|
|
||
| `eit.global_impedance` sums the pixels with `m3resp.eit.processing.global_impedance`, a copy of eitprocessing's `EITData.get_summed_impedance` that gives the same values. |
There was a problem hiding this comment.
This is not a 1-1 translation. Summed impedance is not always the same as global impedance. If the EIT pixel data is masked, it is functional or regional impedance.
There was a problem hiding this comment.
Agreed. The CHANGELOG now says that global_impedance gives the same values as get_summed_impedance only for images without a mask.
Masked images give an error, because their sum is a regional impedance. See my reply on impedance.py for the change itself.
Thanks for your comment! On the folder layout: I think this layout fits m3resp. A file reader or a calculation that only applies to one measurement stays with that measurement. Someone who works on EIT finds the Draeger reader, the breath detection and the lung space methods under This keeps the option of general use open. When a function in |
Part of the confusion comes from the temporary state of the code. Before this PR is ready for review, we will remove On the loaders: the session already reads files with the m3resp readers ( On the split: You are right that We will remove the adapter. The golden tests compare with stored eitprocessing 1.8.7 results. The tests that compare directly with eitprocessing stay, and eitprocessing becomes an optional install for tests only. The CHANGELOG lists each case where m3resp gives a different result on purpose. Yes, the final object is
On the docs: the tutorial script alone finishes in about 3 s on my computer, so the calculations are probably not the cause. The build saves the figure of this tutorial as a PNG through kaleido and Chrome. |
…on masked images EITFileData held one path. A joined recording kept only the first file, and join_eit_files needed a second list of paths for its error messages. Now EITFileData.paths lists every file in recording order, also after a join or a cut. The eitprocessing Sequence built from it gets the same list. join_eit_files is renamed to join_eit_file_data and takes only the file data. The sum of a masked image covers only the region of the mask. On such images, global_impedance gave a regional impedance under a global name. PixelMask.apply now lists the mask name under metadata["masks"] of a masked Signal, also through PixelMaskCollection. global_impedance raises an error when this list is not empty and points to regional_impedance.
…l impedance after its mask - Two regions summed with regional_impedance both got the name regional_impedance, so their waveforms looked the same in a plot or an export. Without a name, the result is now named after the mask, for example ventral_impedance. The channel stays regional_impedance. - The slicing functions used start and end for seconds in some places and for frame numbers in others, so a call could pass one where the other was meant. slice_events, interval_indices, slice_intervals, slice_by_time and time_slice_indices now take start_time and end_time. slice_file_data and slice_by_index take start_index and end_index. The step eit.slice_signal keeps start and end, because its mode setting says which one they are.
…rison tests All EIT steps run on m3resp code, so the install needs no eitprocessing git branch. pip install m3resp reads, processes and plots EIT data. matplotlib is a main dependency for the EIT figures, and the eit option installs plotly only. EITProcessingAdapter moved to tests/reference/eitprocessing. The new reference option installs eitprocessing 1.8.7 from PyPI, and the tests with the pytest marker reference compare m3resp with it. A new CI job runs them. tests/test_eit_without_eitprocessing.py runs the EIT steps with eitprocessing hidden. EITRecording holds the m3resp data only, and its fields data, raw and global_impedance are removed. The loader name "eitprocessing" is now "vendor". A custom loader is given with eit_loader= and returns an EITFileData. The eit_adapter argument and the eit_sequence outputs are removed. session.preprocess_eit() calls the new preprocess_eit_recording and gives the same values as before. The real data tests read the airway pressure with the m3resp reader. Without eitprocessing, only the 3 direct comparisons with it skip.
…install A NaN breathing rate or heart rate went from preprocess into the MDN filter, because only the eit.mdn_filter step checked for it. The rate check of the filter now refuses NaN and infinite rates for every caller. With custom breath settings, the pixel steps ran a second breath detection with the default settings on every call. They now take these breaths from the breath cache of the session, once per signal. The reference marker went on every test of a file that named eitprocessing anywhere, and the marked tests ran when eitprocessing was missing. A test now gets the marker when its file, its own code or one of its fixtures uses eitprocessing, and it skips when eitprocessing is missing. When eit_adapter was removed, ventilator_adapter moved up one place, so a call with positional arguments gave values to the wrong argument. Every argument of M3Session must now be given by name. The help pages, the issue templates and some docstrings still asked for the eitprocessing version or named the removed EIT adapter.
…ng in watershed methods
Summary
This PR moves the EIT analysis from eitprocessing 1.8.7 into m3resp (stage 3). m3resp reads the EIT files and runs all EIT steps with its own code and data types. eitprocessing is installed only for the comparison tests:
.bin, Sentec.zriand Timpel.txtfiles (m3resp.eit.io)Signals, device marks asEventData, breaths asBreathEvents, and native EIT slicingpreprocess_eit_recordingforsession.preprocess_eit()PixelBreaths), pixel TIV and the new pixel EELI ([Feature]: Pixel-level EELI #114, BreathEvents should support pixel-level breaths #136)PixelMapandPixelMaskmethods, geometric masks, the TIV, amplitude and watershed lung-space methods and the size filter (m3resp.eit.roi)json.dumpscan writepip install m3respreads, processes and plots EIT data. matplotlib is a main dependency, and theeitextra installs plotly only. The newreferenceextra installs eitprocessing 1.8.7 from PyPI for the comparison tests.Each ported file has a provenance block that names the original eitprocessing files. The CHANGELOG lists each change a user can see, including the cases where m3resp gives a different result from eitprocessing.
Type of change
Scope
How Has This Been Tested?
tests/regression/golden/eit/.tests/regression/test_eit_golden_reference.pycompares the m3resp results with them. Shapes, counts, indices and masks must be equal. Other values may differ by at most 1e-9 times the largest absolute value in the reference array. NaN must be at the same places. All 52 tests pass locally with eitprocessing 1.8.7.referenceand skip without eitprocessing. The new CI job "Comparison with eitprocessing" installs thereferenceextra and runs them. The oldEITProcessingAdapteris now test code intests/reference/eitprocessing/.tests/test_eit_without_eitprocessing.pyruns the EIT steps with eitprocessing hidden. The optional-deps CI job checks the installed packages of each extra,referenceincluded.test_multimodal_full_example_runs_end_to_end, an EMG breath count (13 found, 14 expected).pytest -m real_data): 20 passed with eitprocessing. Without it, 17 pass and the 3 direct comparisons with eitprocessing skip.sphinx-build -Wand eitprocessing blocked, and the tutorials run.Checks
Notes
continuous_tiv,eeliandpixel_tiv, theroi_masktype, and theeit_sequenceoutputs and inputs.EITRecordingholds the m3resp data only. The loader name"eitprocessing"is now"vendor", and a custom loader is given witheit_loader=in place ofeit_adapter=. The CHANGELOG has the full list.