Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

037 implement eeli wbaccinelli #42

Merged
merged 31 commits into from
Apr 18, 2024
Merged

Conversation

wbaccinelli
Copy link
Collaborator

@wbaccinelli wbaccinelli commented Apr 17, 2024

Implemented EELI

Fixes: #37

@wbaccinelli wbaccinelli requested a review from DaniBodor April 17, 2024 15:02
@@ -24,7 +24,7 @@ repository = "[email protected]:EIT-ALIVE/eit_dash"
python = ">=3.10,<3.13"
dash = { extras = ["testing"], version = "^2.11.1" }
dash-bootstrap-components = "^1.4.1"
eitprocessing = { git = "https://github.com/EIT-ALIVE/eitprocessing" }
eitprocessing = { git = "https://github.com/EIT-ALIVE/eitprocessing", rev = "186_parameter_eeli_psomhorst" }
Copy link
Member

Choose a reason for hiding this comment

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

How confident are we that this branch remains stable?

@DaniBodor
Copy link
Member

DaniBodor commented Apr 18, 2024

I got an error when switching from a lowpass to a bandstop filter in the dropdown menu. If it's just that filter that's not working, maybe remove it for now? It doesn't stop me from proceeding though.

Traceback (most recent call last):
File "/home/dbodor/git/EIT-ALIVE/eit_dash/eit_dash/callbacks/preprocessing_callbacks.py", line 570, in enable_apply_button
and co_low > 0
TypeError: '>' not supported between instances of 'NoneType' and 'int'

@DaniBodor
Copy link
Member

DaniBodor commented Apr 18, 2024

PRE-PROCESSING page

  • Really nice that the preprocessing page now also has info on the selected regions in the summary section.
  • The "Data filtered" box under Results is not removable and only 1 filter can be stored. I think it's overwritten when a new one is stored. It's also not associated to any data, but is just the filter itself, is that on purpose? If so, it should be called "Filter" rather than "Data filtered"

ANALYZE page

  • When I click "Apply EELI", I get an empty dropdown box and no options to do anything.

Error message (when entering ANALYZE):

Traceback (most recent call last):
File "/home/dbodor/git/EIT-ALIVE/eit_dash/eit_dash/callbacks/analyze_callbacks.py", line 49, in page_setup
filter_params = period.get_data().continuous_data.data["global_impedance_filtered"].parameters
KeyError: 'global_impedance_filtered'

@wbaccinelli
Copy link
Collaborator Author

I got an error when switching from a lowpass to a bandstop filter in the dropdown menu. If it's just that filter that's not working, maybe remove it for now? It doesn't stop me from proceeding though.

Traceback (most recent call last):
File "/home/dbodor/git/EIT-ALIVE/eit_dash/eit_dash/callbacks/preprocessing_callbacks.py", line 570, in enable_apply_button
and co_low > 0
TypeError: '>' not supported between instances of 'NoneType' and 'int'

fixed here

@wbaccinelli
Copy link
Collaborator Author

wbaccinelli commented Apr 18, 2024

PRE-PROCESSING page

  • Really nice that the preprocessing page now also has info on the selected regions in the summary section.
  • The "Data filtered" box under Results is not removable and only 1 filter can be stored. I think it's overwritten when a new one is stored. It's also not associated to any data, but is just the filter itself, is that on purpose? If so, it should be called "Filter" rather than "Data filtered"

Yes, this is on purpose and it filters all the periods. Only one type of filter can be applied at the moment and if a new filter is applied, it will override the previous results. I used "Data filtered", because the result is that the data have been filtered with the parameters reported, but I don't mind changing it,

ANALYZE page

  • When I click "Apply EELI", I get an empty dropdown box and no options to do anything.

Error message (when entering ANALYZE):

Traceback (most recent call last):
File "/home/dbodor/git/EIT-ALIVE/eit_dash/eit_dash/callbacks/analyze_callbacks.py", line 49, in page_setup
filter_params = period.get_data().continuous_data.data["global_impedance_filtered"].parameters
KeyError: 'global_impedance_filtered'

Did you get this after applying the filter? This makes me think that I have to disable the option if no filtered data are available, or I need to use the raw data if the filtered data are not there.

Copy link
Member

@DaniBodor DaniBodor left a comment

Choose a reason for hiding this comment

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

Great job!

I added 2 minor comments about the user manual. Also I noticed a typo that I made in that document at line 44, where Click is spelled with a capital and shouldn't be. Could you fix that here as well, please.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note at the very top asking people to open issues if they encounter anything?

docs/user_manual.md Show resolved Hide resolved
@wbaccinelli wbaccinelli merged commit 8b1826e into main Apr 18, 2024
1 of 2 checks passed
@wbaccinelli wbaccinelli deleted the 037_implement_eeli_wbaccinelli branch June 27, 2024 11:25
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.

Implement EELI analysis
2 participants