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

Better error message upon failure in loading OpenEphysBinary folder #1389

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alejoe91
Copy link
Contributor

Something I've been wanting to do for a long time :)

@alejoe91 alejoe91 added this to the 0.13.0 milestone Jan 31, 2024
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

It's a big diff, but really you just wrapped with a try-except right?

neo/rawio/openephysbinaryrawio.py Show resolved Hide resolved
zm711
zm711 previously approved these changes Jan 31, 2024
Copy link
Contributor

@zm711 zm711 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@samuelgarcia
Copy link
Contributor

I think I am clearly not agree with this.
This big try except is not good design.
We need to put try except on very targeted places that focus on a clear failure that could happen.

@alejoe91
Copy link
Contributor Author

alejoe91 commented Feb 1, 2024

Better than before at least. I agree that long term we need a better handling, but I'm not sure when I'll have time for it

@zm711
Copy link
Contributor

zm711 commented Feb 1, 2024

I also agree more targeted would be better, but at least what I see from the tracker the usual flow is this.

I have an error with openephysbinary (gives error)
That's probably related to folder structure could you list the files.

With this error you tell them that it is likely the folder structure to begin with. Maybe a comment to say in the future improve to more specific/more helpful errors (ie this file was not found--I don't know openephy very well so I'm not sure what the structure would really need to be).

@samuelgarcia
Copy link
Contributor

At least lets do the try in the other part:

try:
    explore_folder(dirname, experiment_names=None)
except:
   ...

No ?

@alejoe91
Copy link
Contributor Author

alejoe91 commented Feb 2, 2024

At least lets do the try in the other part:

try:
    explore_folder(dirname, experiment_names=None)
except:
   ...

No ?

The second part of the explore_folder already throws an interpretable errror about not finding any structure.oebin files. Only the first part tends to fail with un-interpretable errors

@apdavison apdavison modified the milestones: 0.13.0, 0.13.1 Feb 2, 2024
@zm711 zm711 dismissed their stale review February 2, 2024 18:05

neo meeting

@alejoe91 alejoe91 modified the milestones: 0.13.1, 0.14.0 Apr 5, 2024
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