Skip to content

Feat/stage3 eit integration - #148

Open
Mahyar-t wants to merge 18 commits into
mainfrom
feat/stage3_eit_integration
Open

Mahyar-t wants to merge 18 commits into
mainfrom
feat/stage3_eit_integration

Conversation

@Mahyar-t

@Mahyar-t Mahyar-t commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  • Readers for Draeger .bin, Sentec .zri and Timpel .txt files (m3resp.eit.io)
  • EIT images and waveforms as Signals, device marks as EventData, breaths as BreathEvents, and native EIT slicing
  • Respiratory and heart rate detection and breath detection
  • MDN and Butterworth filters, and preprocess_eit_recording for session.preprocess_eit()
  • Global TIV and EELI, also on breaths of another modality (Adapt eitprocessing functions to accept pre-determined breaths #113)
  • Pixel breaths (new type PixelBreaths), pixel TIV and the new pixel EELI ([Feature]: Pixel-level EELI #114, BreathEvents should support pixel-level breaths #136)
  • The PixelMap and PixelMask methods, geometric masks, the TIV, amplitude and watershed lung-space methods and the size filter (m3resp.eit.roi)
  • Pixel-map figures, the plot style of each map kind and plot data for the GUI that json.dumps can write
  • pip install m3resp reads, processes and plots EIT data. matplotlib is a main dependency, and the eit extra installs plotly only. The new reference extra 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Scope

  • M3Resp integration/API
  • EIT adapter or EIT workflow
  • EMG adapter or EMG workflow
  • Synchronization/export
  • Documentation
  • Tests/CI

How Has This Been Tested?

  • Golden reference tests. Before the port, I saved the eitprocessing 1.8.7 results for three test files, one per vendor (Draeger, Sentec, Timpel), in tests/regression/golden/eit/. tests/regression/test_eit_golden_reference.py compares 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.
  • Unit tests for each ported function: readers, rates, breaths, filters, TIV and EELI, pixel breaths, lung space, pixel maps, figures and plot data.
  • Comparison tests. The tests that compare m3resp with eitprocessing get the pytest marker reference and skip without eitprocessing. The new CI job "Comparison with eitprocessing" installs the reference extra and runs them. The old EITProcessingAdapter is now test code in tests/reference/eitprocessing/.
  • tests/test_eit_without_eitprocessing.py runs the EIT steps with eitprocessing hidden. The optional-deps CI job checks the installed packages of each extra, reference included.
  • Workflow tests for the EIT steps, their inputs and outputs, and the workflow checks.
  • Full test suite: 4612 passed and 1 failed. The failure is test_multimodal_full_example_runs_end_to_end, an EMG breath count (13 found, 14 expected).
  • Real-data tests (pytest -m real_data): 20 passed with eitprocessing. Without it, 17 pass and the 3 direct comparisons with eitprocessing skip.
  • The docs build passes with sphinx-build -W and eitprocessing blocked, and the tutorials run.

Checks

  • Documentation updated if behavior changed
  • Upstream package behavior is preserved
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Notes

  • The PR is large (226 files). Most of the added lines are tests and golden files. Each commit is one port step, so reviewing commit by commit is the easiest way through it.
  • Breaking changes: some step outputs, inputs and artifact types changed or were removed. Examples are the eitprocessing outputs continuous_tiv, eeli and pixel_tiv, the roi_mask type, and the eit_sequence outputs and inputs. EITRecording holds the m3resp data only. The loader name "eitprocessing" is now "vendor", and a custom loader is given with eit_loader= in place of eit_adapter=. The CHANGELOG has the full list.
  • "Upstream package behavior is preserved" holds for the test files at the stored settings. The CHANGELOG lists each case where m3resp gives a different result, for example a 0 Hz rate limit, pixel breaths at the edge of the data, and NaN pixels in the watershed method.
  • Still open: the filter-spectrum figure and the contour and 3D pixel maps. The fixes for the eitprocessing issues found during the port come in a separate PR.
  • The progress report I shared in Teams maps each eitprocessing file, class and function to its m3resp counterpart. It helps you find the new place of a function you know from eitprocessing.

….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.
@Mahyar-t Mahyar-t self-assigned this Oct 31, 2026

@psomhorst psomhorst left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +77 to +78
("draeger", "global_impedance_(raw)"): "impedance",
("timpel", "global_impedance_(raw)"): "global_impedance",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these differ? They are created in the same way, i.e., by summing the pixel impedance (EITData.calculate_global_impedance())

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/io/draeger.py Resolved
Comment thread src/m3resp/eit/io/draeger.py Resolved
Comment thread src/m3resp/eit/io/draeger.py Resolved
Comment thread src/m3resp/eit/io/draeger.py Resolved
Comment thread src/m3resp/eit/io/timpel.py Resolved
Comment thread src/m3resp/eit/io/timpel.py Outdated
Comment on lines +176 to +178
# The value is kept as given: the default is the whole number 50, as in
# eitprocessing.
frequency = sample_frequency

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Mahyar-t Mahyar-t Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/io/timpel.py Resolved
Comment thread src/m3resp/eit/io/timpel.py Resolved
Comment thread src/m3resp/eit/io/timpel.py Resolved

@psomhorst psomhorst left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +130 to +134
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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Mahyar-t Mahyar-t Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/io/join.py Outdated
GAP_TOLERANCE_FRAMES = 0.5


def join_eit_files(files: Sequence[EITFileData], paths: Sequence[str]) -> EITFileData:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/io/join.py Outdated

Returns:
EITFileData: All frames, waveforms, marks and breaths in one
recording. ``path`` is the path of the first file.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I don't agree with the workflow this file suggests.

The workflow should generally be:

  1. other preprocessing steps
  2. create mask
  3. apply mask to pixel data
    1. sum to get regional / functional data
    2. 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.

@Mahyar-t Mahyar-t Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/processing/impedance.py Outdated
Comment on lines +83 to +84
name: str = "regional_impedance",
channel: str = "regional_impedance",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@Mahyar-t Mahyar-t Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/eit/processing/slicing.py Outdated
Comment on lines +119 to +120
start (float): Start of the window, in seconds.
end (float): End of the window, in seconds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/m3resp/processing/slicing.py Resolved
Comment thread src/m3resp/processing/slicing.py Outdated
"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.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
raise ValueError("Slicing bounds must be finite times in seconds.")
raise ValueError("Slicing bounds must be finite times in seconds or None.")

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Comment thread CHANGELOG.md Outdated

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Mahyar-t

Mahyar-t commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

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.

Thanks for your comment! On the folder layout: m3resp/eit follows the convention Chiara also uses in #149, so EIT and EMG have the same structure (eit/io and eit/processing, emg/io and emg/processing).

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 m3resp/eit. Code that more than one measurement can use goes in m3resp/processing. The EIT code already imports from there: peak detection, the moving average and the cutting of a signal by index.

This keeps the option of general use open. When a function in m3resp/eit turns out to be useful for another measurement, we move it to m3resp/processing. I would keep the layout for now and look at it again if more code turns out to be shared. If you see a specific function that should be general, tell me and we move it.

@Mahyar-t

Mahyar-t commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

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)

Part of the confusion comes from the temporary state of the code. Before this PR is ready for review, we will remove EITProcessingAdapter and the eitprocessing dependency from the package. All EIT calculations already run in m3resp code.

On the loaders: the session already reads files with the m3resp readers (read_eit_file), also at fc20aef. It then builds an eitprocessing Sequence from that data, so eitprocessing must still be installed, and the name load_with_eitprocessing is misleading. We will remove the Sequence before converting this draft PR to the final PR, so the session will use only the m3resp readers.

On the split: eit/io reads and joins the files. modalities/eit.py will hold the session recording and the functions that cut it in time.

You are right that modalities/eit.py is messy. The recording holds its data twice, and load has three paths to read a file. Both come from the Sequence and the adapter, and both go when those go and mess will go away before the final PR.

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 session.signals. preprocess_eit adds the raw and filtered global impedance there through _extend_typed_collections_from_eit. The filtered images currently go only to session.processed, while workflows add them to session.signals. We will change preprocess_eit to add them too, so both routes give the same contents.

EITFileData is the output of a reader, and it can be joined and cut. EITRecording is the start of the processing, and preprocess_eit reads from it. Its only calculated data is the raw global impedance, the sum of the pixels. Once the Sequence is gone, the two objects hold almost the same fields, so yes, merging them is a sensible follow-up.

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.
@Mahyar-t
Mahyar-t marked this pull request as ready for review November 10, 2026 08:52
…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.

This branch has not been deployed

No deployments
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