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

Removing *subject_ids* argument in TUHAbnormal example. #402

Merged

Conversation

bruAristimunha
Copy link
Collaborator

Related with the issue #377.

Removing a deprecated parameter in the TUHAbnormal example.

@codecov
Copy link

codecov bot commented Aug 27, 2022

Codecov Report

Merging #402 (62520ed) into master (d0abec3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   84.20%   84.20%           
=======================================
  Files          46       46           
  Lines        3616     3616           
=======================================
  Hits         3045     3045           
  Misses        571      571           

@robintibor
Copy link
Contributor

So is this correct @gemeinl ?

@gemeinl
Copy link
Collaborator

gemeinl commented Sep 12, 2022

No, actually not correct.
The function load_example_data has an argument subject_ids which does not have any effect anymore, due to the changes introduced here. Therefore, the entire dataset will be loaded instead of just 10 recs.
We still need this argument, however, its meaning has changed. It is not subjects we are refering to but recordings. I suggest to rename the argument to n_recordings and give this as input to TUHAbnormal which has the an argument recording_ids (see

def __init__(self, path, recording_ids=None, target_name='pathological',
preload=False, add_physician_reports=False, n_jobs=1):
).

@bruAristimunha
Copy link
Collaborator Author

Many thanks for the explication @gemeinl. I was wondering, can you review it again to make sure everything is correct?

@gemeinl
Copy link
Collaborator

gemeinl commented Sep 14, 2022

I suggest to stick as close to the original code as possible.
So change the input argument n_subjects to n_recordings in load_example_data and create the recording_ids for TUHAbnormal inside as before.

@bruAristimunha
Copy link
Collaborator Author

bruAristimunha commented Sep 14, 2022

Done @gemeinl. Can you run it once? I don't have this dataset.

Copy link
Collaborator

@gemeinl gemeinl left a comment

Choose a reason for hiding this comment

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

Great, thanks a lot @bruAristimunha !

@bruAristimunha
Copy link
Collaborator Author

Can you merge? @robintibor

@cedricrommel
Copy link
Collaborator

Could you solve the conflicts @bruAristimunha before I merge?

@bruAristimunha
Copy link
Collaborator Author

Done @cedricrommel.

@cedricrommel cedricrommel merged commit eb9ccd1 into braindecode:master Sep 30, 2022
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.

None yet

4 participants