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

readers.py unbound variable #340

Closed
mulhod opened this issue Feb 27, 2017 · 7 comments
Closed

readers.py unbound variable #340

mulhod opened this issue Feb 27, 2017 · 7 comments

Comments

@mulhod
Copy link
Contributor

mulhod commented Feb 27, 2017

In the following section ex_num might not ever be assigned to if the features file contains no entries at all. This is obviously a problem for the experiment, but the failure should perhaps be handled better, maybe hinting at the issue since it's not clear until you look into it a bit further.

  185         with open(self.path_or_list, 'r' if PY3 else 'rb') as f:                                      x: 'response_id,cva2,y\n'
  186             for ex_num, (id_, class_, _) in enumerate(self._sub_read(f), start=1):                    
  187                 # Update lists of IDs, clases, and features                                           
  188                 if self.ids_to_floats:                                                                Stack:
  189                     try:                                                                              >> read [CSVReader] readers.py:201
  190                         id_ = float(id_)                                                                 _load_featureset experiments.py:392
  191                     except ValueError:                                                                   _classify_featureset experiments.py:496
  192                         raise ValueError(('You set ids_to_floats to true,'                               run_configuration experiments.py:1034
  193                                           ' but ID {} could not be '                                     main run_experiment.py:108
  194                                           'converted to float in '                                       <module> run_experiment.py:112
  195                                           '{}').format(id_,                                           
  196                                                        self.path_or_list))                            
  197                 ids.append(id_)                                                                       
  198                 labels.append(class_)                                                                 
  199                 if ex_num % 100 == 0:                                                                 
  200                     self._print_progress(ex_num)                                                      
  201             self._print_progress(ex_num)
@mulhod
Copy link
Contributor Author

mulhod commented Feb 28, 2017

It turns out that this issue was previously raised (then closed without fixing): #236. Additionally, #253 touched on this issue but didn't resolve it completely. Maybe it would be best to open #236 again and close this issue.

@aoifecahill
Copy link
Collaborator

ah, yes, that's my fault. When fixing #253 I only added the start=1 clause to one of the enumerates. I think that was supposed to address the fact that ex_num was never explicitly instantiated.

@desilinguist desilinguist modified the milestone: 1.5 Aug 18, 2017
@desilinguist
Copy link
Member

I am going to fix this:

  1. I will initialize ex_num to 0 outside of the loop.
  2. I will add a warning if ex_num stayed at 0 once the loop was over, indicating that the given file is possibly empty. I will also return an empty FeatureSet instance in this case.

Does that sound right @mulhod @aoifecahill ?

@aoifecahill
Copy link
Collaborator

Yes, that sounds good to me.

@mulhod
Copy link
Contributor Author

mulhod commented Nov 13, 2017

Sounds good!

@desilinguist
Copy link
Member

Actually, based on our in-person discussion, it's better to just raise an exception if we encounter an empty file since there's no conceivable scenario in which we would knowingly read in an empty file into a featureset instance.

@desilinguist
Copy link
Member

Addressed by #392.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants