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

BF: make data.importConditions() able to import csv with ',' and ';' separator #2825

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

AnnaTruzzi
Copy link
Contributor

In the data.importConditions() (lines 276-289) I added a try/except block to allow an automatic loading of csv with either ',' or ';' as separator.
Now, pandas loads the file without raising errors even if only one column is detected (for example when loading a file separated with semicolon with the argument sep = ',') and the presence of weird punctuation in the dataset is only tested later when the _assertValidVarNames() function is called from within the pandasToDictList() function. Therefore the except function only works if pandasToDictList(trialsArr) is run before it.
Right now to avoid changing the code too much I have repeated the same block of code twice (and it is used one more time later on for the excel format). It works but it seems to me a bit redundant.
What do you think?

@coveralls
Copy link

coveralls commented Mar 5, 2020

Coverage Status

Coverage decreased (-0.008%) to 48.396% when pulling 4eda564 on AnnaTruzzi:master into 5df405c on psychopy:master.

@peircej
Copy link
Member

peircej commented Mar 5, 2020

I totally hear you. It's tricky. Two things could do with improving ideally :

  • the amount of code inside the try/except. The more lines we have in a block like that the more risk we have of trapping also trapping a different (unrelated) ValueError in the future and confusing ourselves by the lack of a message
  • the repetition of those lines. The issue then is that if we need to fix the we have to fix in 3 places

Possible solutions:

  • make those calls in a loop for sep in [',', ';']:. Use try/except just around the critical line as in:
  try:
    lineMightFail()
    break  # it didn't so end the loop
  except ValueError:
    continue  # it failed so try other option
  • make those lines into a private function (as we did with pandasToDictList) and then we can call it multiple times. The try/except could just go around the line that fails inside the private function and it could return a value to indicate that type of failure (or we could create a custom Exception class if we're being fancy)

Sorry, this has turned into a rabbit hole. I'm happy to take the version you submitted already - it does fix the bug and working inelegant code is better than broken elegant code! Up to you whether you want to pretty-it-up further.

@peircej
Copy link
Member

peircej commented Mar 6, 2020

Hi Anna, I thnk I'm going to pull this in and then adapt it further if that's OK. I've realised that there is a further issue not handled yet which is that the decimal separator is also going to be variable the reason your spreadsheet uses ; as its delimiter is almost certainly that it's using , as the decimal separator. So I think we do need to switch to using a loop (because there are several possible options now) and I think I will put the code into a separate private function. So I'll share the changes once complete.

@peircej peircej merged commit 48172aa into psychopy:master Mar 6, 2020
@AnnaTruzzi
Copy link
Contributor Author

Sounds great!

peircej added a commit to peircej/psychopy that referenced this pull request Mar 6, 2020
Taking Anna's fix psychopyGH-2825 a step further:

- also handle the decmial separator (likely to be , if sep is ; )
- also handle tab separator
- also handle other  file extensions (tsv and dlm are possible)
- added tests for these various files
peircej added a commit that referenced this pull request Mar 6, 2020
Taking Anna's fix GH-2825 a step further:

- also handle the decmial separator (likely to be , if sep is ; )
- also handle tab separator
- also handle other  file extensions (tsv and dlm are possible)
- added tests for these various files
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.

3 participants