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

Add try/except to np.polyfit #380

Merged
merged 1 commit into from
Aug 14, 2023
Merged

Add try/except to np.polyfit #380

merged 1 commit into from
Aug 14, 2023

Conversation

dpdutcher
Copy link
Collaborator

np.polyfit can fail for some complicated reasons, and it's better to skip a problematic channel than to fail the whole algorithm. I think a try/except here is the easiest solution.

I also don't know all the failure modes (TypeError, SystemError, etc.), so I left it as a generic except.

np.polyfit can fail for some complicated reasons, and it's better to skip a problematic channel than to fail the whole algorithm. I think a try/except here is the easiest solution.

I also don't know all the failure modes (TypeError, SystemError, etc.), so I left it as a generic except.
@dpdutcher dpdutcher requested review from jlashner and msilvafe August 14, 2023 17:49
Copy link
Collaborator

@jlashner jlashner left a comment

Choose a reason for hiding this comment

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

Thanks daniel, currently looking into why this is failing, but this seems like a necessary change for the time being.

@dpdutcher dpdutcher merged commit 5a579b5 into master Aug 14, 2023
@dpdutcher dpdutcher deleted the polyfit_bugfix branch August 14, 2023 17:52
@msilvafe
Copy link
Contributor

Should we leave this except in here even after Jack tracked down the cause and merged the fix?

@jlashner
Copy link
Collaborator

I think it's good to keep this, we really don't want the analysis function to fail out if we can avoid it, since then you lose the iva object. Its much easier to inspect bad channels and failures if the function finishes and returns the iva object

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