222 pixel inflation #241

JulietteFrancovich · 2024-06-14T12:40:13Z

No description provided.

JulietteFrancovich · 2024-09-16T10:10:06Z

@psomhorst this branch now contains the feature pixel inflations and the parameter TIV since they are interdependent. I have written tests for pixel inflations. Could you review the code and give your suggestions for pixel inflations and the tests? Once we are happy with that I will write the tests for parameter TIV and request review again for the entire pull request. Thanks!

psomhorst

I think this is a great PR with a lot of work and thought in it. There are some things I would improve, but overall it seems almost good to go.

The algorithm is quite slow. I would like to look into trying to speed it up. I have a suggestion on how to prevent iterating over all pixels. Maybe this will help in the performance.

Please make sure the code is formatted using "ruff < 0.6". I will update the main branch to use the last version of ruff before 0.6 (currently 0.5.7).

I have only reviewed pixel inflation and its test. I have not reviewed the TIV parameters class or the notebooks.

eitprocessing/features/pixel_inflation.py

tests/test_pixel_inflation.py

JulietteFrancovich · 2024-09-23T14:57:15Z

@psomhorst Thank you for your review. I have addressed your comments and tried to incorporate your suggestions (see comments). Please let me know if you have further comments/ concerns. I will start working on tests for parameter TIV so we can review that next.

JulietteFrancovich · 2024-09-27T12:55:41Z

@psomhorst I have now written tests for parameter TIV. Could you review the TIV code and tests? Once those have been reviewed and the comments (also on pixel inflation) have been resolved to your liking, I think this branch is ready for merging.

psomhorst

I have some additional comments about the PixelInflation tests and notebook.

tests/test_pixel_inflation.py

eitprocessing/features/pixel_inflation.py

tests/test_pixel_inflation.py

tests/test_parameter_tiv.py

notebooks/test_pixel_inflation.ipynb

psomhorst

This generally looks very good. I notice you're not working with multidimensional data very efficiently. I have some suggestions improving performance and readability.

eitprocessing/parameters/tidal_impedance_variation.py

psomhorst

I have some more comments.

tests/test_parameter_tiv.py

eitprocessing/features/pixel_inflation.py

tests/test_parameter_tiv.py

eitprocessing/parameters/tidal_impedance_variation.py

psomhorst

I think you're almost there!

I think TIV.compute_parameter() return a SparseData object. TIVs are values occurring at unpredictable intervals. For pixels, I think values should be a list of arrays.

There are some very minor issues that result in warnings. These should be fixed before merging.

I have some left over suggestions to improve readability of the code.

Documentation is not entirely there yet, but it's good enough for now. I think we should do a review of all docstrings at some point and try to find a good mode for explaining features.

Having the results as lists of arrays is not ideal. I suggest we leave that for now and reconsider how to do this in the near future. I created #309.

eitprocessing/parameters/tidal_impedance_variation.py

eitprocessing/features/pixel_breath.py

eitprocessing/parameters/tidal_impedance_variation.py

JulietteFrancovich · 2024-10-17T10:08:54Z

@psomhorst I have addressed your comments and changed the TIV to be stored as SparseData. I have adjusted the pixel breath accordingly (since this uses TIV) and have added new tests to check the storage of TIV. Let me know if you have any further comments :)

Co-authored-by: Peter Somhorst <127968566+psomhorst@users.noreply.github.com>

…atory method

expected to work

… array to fit current IntervalData

…ise objects directly

…ction and tests accordingly.

psomhorst

Fantastic! Great effort, thanks for the change to SparseData.

I took the liberty to rebase the branch onto develop, so you can go ahead and merge it.

JulietteFrancovich · 2024-10-17T11:28:29Z

@psomhorst Thank you! I have merged and deleted the branch.

Update documentation for #241

JulietteFrancovich requested a review from psomhorst June 14, 2024 12:40

JulietteFrancovich assigned JulietteFrancovich and nllupgens Jun 14, 2024

psomhorst force-pushed the 041c_breath_detection branch from 047968d to bded081 Compare June 25, 2024 09:31

psomhorst force-pushed the 041c_breath_detection branch from a407481 to dc73f52 Compare July 5, 2024 08:14

Base automatically changed from 041c_breath_detection to develop July 9, 2024 08:02

psomhorst force-pushed the 222_pixel_inflation branch from 5978134 to 2fbce72 Compare July 9, 2024 09:49

JulietteFrancovich force-pushed the 222_pixel_inflation branch from 2fbce72 to 0d07a73 Compare July 26, 2024 09:15

JulietteFrancovich marked this pull request as draft July 26, 2024 10:53

psomhorst removed their request for review July 26, 2024 12:29

JulietteFrancovich force-pushed the 222_pixel_inflation branch from f2d742c to 03a63d9 Compare July 26, 2024 14:04

psomhorst marked this pull request as ready for review September 20, 2024 09:41

psomhorst self-requested a review September 20, 2024 09:41

psomhorst reviewed Sep 20, 2024

View reviewed changes

psomhorst reviewed Sep 30, 2024

View reviewed changes

psomhorst requested changes Oct 3, 2024

View reviewed changes

psomhorst mentioned this pull request Oct 8, 2024

Allow multi-dimensional data in IntervalData #304

Open

psomhorst added this to the Next release: v1.4.0 milestone Oct 8, 2024

psomhorst self-requested a review October 8, 2024 06:54

psomhorst reviewed Oct 8, 2024

View reviewed changes

JulietteFrancovich requested a review from psomhorst October 9, 2024 14:48

psomhorst requested changes Oct 10, 2024

View reviewed changes

psomhorst reviewed Oct 10, 2024

View reviewed changes

eitprocessing/parameters/tidal_impedance_variation.py Outdated Show resolved Hide resolved

JulietteFrancovich requested a review from psomhorst October 17, 2024 10:07

psomhorst added 2 commits October 17, 2024 13:18

Add temporary DetectBreaths class, NB: REMOVE LATER!

fae5aa0

Add notebook to test EELI functionality

fe425e1

JulietteFrancovich and others added 22 commits October 17, 2024 13:18

updated fixtures

950e8e8

created tests for parameter TIV

09c023c

updated docstrings

a2d3c08

updated notebooks

be14eac

Apply suggestions from code review

c600201

Co-authored-by: Peter Somhorst <127968566+psomhorst@users.noreply.github.com>

made error messages more consistent and incorporated np.diff in expir…

ec31a9a

…atory method

removed unnecessary iteration over breaths from TIV computation

74f13d0

added test for different bd_kwargs

51bf4d1

Improved tests based on reviewer comments

fdb8c8f

changed shape[0] to len()

20f2a5b

added function to initialize array for parametrization

dfd7b04

separated tests expected to throw errors from tests that are

0b6053b

expected to work

save pixel inflation results as a list of 2D-arrays, rather than a 3D…

3e37322

… array to fit current IntervalData

refactored code to change pixel inflations to pixel breaths

e21a697

moved NotImplementedError for method to initialization

aeb7abd

fixed linting

9c258b9

cleaned up notebooks and added explanation

49ced2b

Updated the tests to represent the actual data structures and initial…

eef3b20

…ise objects directly

added check for mean of empty slices. Renamed start to outsides.

b82a031

changed TIV to be stored as SparseData and adjusted pixel breath dete…

d7a72a7

…ction and tests accordingly.

updated docstrings

29fa1d2

updated notebook to work with TIV stored as SparseData

8ed283b

psomhorst force-pushed the 222_pixel_inflation branch from 57eb8cc to 8ed283b Compare October 17, 2024 11:18

psomhorst approved these changes Oct 17, 2024

View reviewed changes

JulietteFrancovich merged commit 9cecb13 into develop Oct 17, 2024
4 checks passed

JulietteFrancovich deleted the 222_pixel_inflation branch October 17, 2024 11:28

psomhorst added a commit that referenced this pull request Oct 17, 2024

Update documentation for #241

44d3984

psomhorst added a commit that referenced this pull request Oct 17, 2024

Merge pull request #313 from EIT-ALIVE/update_docs

b92f073

Update documentation for #241

psomhorst added a commit that referenced this pull request Oct 17, 2024

Merge pull request #312 from EIT-ALIVE/update_docs

1f98f6d

Update documentation for #241

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

222 pixel inflation #241

222 pixel inflation #241

JulietteFrancovich commented Jun 14, 2024

JulietteFrancovich commented Sep 16, 2024

psomhorst left a comment

JulietteFrancovich commented Sep 23, 2024 •

edited

Loading

JulietteFrancovich commented Sep 27, 2024

psomhorst left a comment

psomhorst left a comment

psomhorst left a comment

psomhorst left a comment

JulietteFrancovich commented Oct 17, 2024

psomhorst left a comment

JulietteFrancovich commented Oct 17, 2024

222 pixel inflation #241

222 pixel inflation #241

Conversation

JulietteFrancovich commented Jun 14, 2024

JulietteFrancovich commented Sep 16, 2024

psomhorst left a comment

Choose a reason for hiding this comment

JulietteFrancovich commented Sep 23, 2024 • edited Loading

JulietteFrancovich commented Sep 27, 2024

psomhorst left a comment

Choose a reason for hiding this comment

psomhorst left a comment

Choose a reason for hiding this comment

psomhorst left a comment

Choose a reason for hiding this comment

psomhorst left a comment

Choose a reason for hiding this comment

JulietteFrancovich commented Oct 17, 2024

psomhorst left a comment

Choose a reason for hiding this comment

JulietteFrancovich commented Oct 17, 2024

JulietteFrancovich commented Sep 23, 2024 •

edited

Loading