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

Update base.py #371

Merged
merged 4 commits into from
Jan 5, 2022
Merged

Update base.py #371

merged 4 commits into from
Jan 5, 2022

Conversation

MohammadJavadD
Copy link
Contributor

@MohammadJavadD MohammadJavadD commented Dec 20, 2021

Add list to acceptable types for target_names. It solves this issue: #370

Add `list' to acceptable type for `target_names`.  It solves this issue: braindecode#370
Copy link
Collaborator

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

can you update an existing test to avoid a future regression and to make sure these new lines are covered by some test?

@@ -113,14 +113,16 @@ def set_description(self, description, overwrite=False):
self._description = pd.concat([self.description, description])

def _target_name(self, target_name):
if target_name is not None and type(target_name) not in [str, tuple]:
raise ValueError('target_name has to be None, str, tuple')
if target_name is not None and type(target_name) not in [str, tuple, list]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be using isinstance

@robintibor
Copy link
Contributor

Thanks @MohammadJavadD for the PR, have tried to address comments and clean up a bit. @gemeinl would be nice if you can check if this makes sense for you or you anticipate any problems?

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #371 (834ee25) into master (ad0ab1a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 834ee25 differs from pull request most recent head 9e7f782. Consider uploading reports for the commit 9e7f782 to get more accurate results

@@            Coverage Diff             @@
##           master     #371      +/-   ##
==========================================
+ Coverage   82.73%   82.74%   +0.01%     
==========================================
  Files          53       53              
  Lines        3718     3721       +3     
==========================================
+ Hits         3076     3079       +3     
  Misses        642      642              

Copy link
Collaborator

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

🙏 can you just add an entry in what's new to document the bug fix?

@gemeinl
Copy link
Collaborator

gemeinl commented Jan 5, 2022

Thanks @MohammadJavadD @robintibor
Please also add list to the accepted types for target_name in load_concat_dataset (

target_name: None or str
Load specific description column as target. If not given, take saved
target name.
)

It appears the usage of target_name in load_concat_dataset is not tested at all. It should be tested of course but can be a separate PR.

@robintibor robintibor merged commit 5b4d8da into braindecode:master Jan 5, 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.

4 participants