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: supposed fix for the gh-439, but still unable to reproduce OP. #447

Merged
merged 7 commits into from Nov 2, 2014

Conversation

Projects
None yet
3 participants
@arokem
Member

arokem commented Oct 12, 2014

This should help to deal with the issue reported here:

#439

I am still unable to reproduce the OP, though. @oesteban : would you mind sharing the data that caused the original error to happen? I would like to add that to the tests so that it will not reoccur. Thanks!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 26, 2014

Did you manage to get the data from Oscar? Any progress with this?

@arokem

This comment has been minimized.

Member

arokem commented Oct 26, 2014

No - I haven't gotten the data from @oesteban yet. I tried to reproduce what he got based on his report, but didn't manage to reproduce it. I am not sure how to trigger that linalg error without nan's

@arokem

This comment has been minimized.

Member

arokem commented Oct 29, 2014

Well - I have not managed to get that data, and I have not found a good case for those LinAlgErrors. In the meanwhile, I have managed to increase test coverage up to 97%, though (all statements in the code except for those LinAlgError clauses). So that's good.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 30, 2014

Okay so what do you want me to do with this one here? Ready to merge? Would you mind testing if having min_signal = 0.0001 or something small would still be okay? I am talking about the issue reported by Rutger. Let me know what you think.

@arokem

This comment has been minimized.

Member

arokem commented Oct 30, 2014

OK - I have also changed the min_signal here to deal with gh-451. Go ahead
and merge if this looks fine to you.

On Thu, Oct 30, 2014 at 11:31 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay so what do you want me to do with this one here? Ready to merge?
Would you mind testing if having min_signal = 0.0001 or something small
would still be okay? I am talking about the issue reported by Rutger. Let
me know what you think.


Reply to this email directly or view it on GitHub
#447 (comment).

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 30, 2014

Just FYI, I've had issues with having the min_signal too low, but I don't
remember exactly how to recreate it. The safest way of dealing with it that
I've been able to come up with has been to use something like min_signal = data[data > 0].min().

On Thu, Oct 30, 2014 at 2:22 PM, Ariel Rokem notifications@github.com
wrote:

OK - I have also changed the min_signal here to deal with gh-451. Go ahead
and merge if this looks fine to you.

On Thu, Oct 30, 2014 at 11:31 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay so what do you want me to do with this one here? Ready to merge?
Would you mind testing if having min_signal = 0.0001 or something small
would still be okay? I am talking about the issue reported by Rutger.
Let
me know what you think.


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Oct 31, 2014

That doesn't seem too bad to me, actually. I can't think of a downside to
it, off the top of my head. Do you suggest we pull out the min_signal
kwarg altogether? Or maybe we set it per default to None?

On Thu, Oct 30, 2014 at 3:54 PM, MrBago notifications@github.com wrote:

Just FYI, I've had issues with having the min_signal too low, but I don't
remember exactly how to recreate it. The safest way of dealing with it
that

I've been able to come up with has been to use something like `min_signal

data[data > 0].min()`.

On Thu, Oct 30, 2014 at 2:22 PM, Ariel Rokem notifications@github.com
wrote:

OK - I have also changed the min_signal here to deal with gh-451. Go
ahead
and merge if this looks fine to you.

On Thu, Oct 30, 2014 at 11:31 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay so what do you want me to do with this one here? Ready to merge?
Would you mind testing if having min_signal = 0.0001 or something
small
would still be okay? I am talking about the issue reported by Rutger.
Let
me know what you think.


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).

@MrBago

This comment has been minimized.

Contributor

MrBago commented Oct 31, 2014

I wouldn't pull it right away just to avoid compatibility issues, None
sounds good to me though.

Bago

On Fri, Oct 31, 2014 at 7:51 AM, Ariel Rokem notifications@github.com
wrote:

That doesn't seem too bad to me, actually. I can't think of a downside to
it, off the top of my head. Do you suggest we pull out the min_signal
kwarg altogether? Or maybe we set it per default to None?

On Thu, Oct 30, 2014 at 3:54 PM, MrBago notifications@github.com wrote:

Just FYI, I've had issues with having the min_signal too low, but I
don't
remember exactly how to recreate it. The safest way of dealing with it
that
I've been able to come up with has been to use something like

`min_signal

data[data > 0].min()`.

On Thu, Oct 30, 2014 at 2:22 PM, Ariel Rokem notifications@github.com
wrote:

OK - I have also changed the min_signal here to deal with gh-451. Go
ahead
and merge if this looks fine to you.

On Thu, Oct 30, 2014 at 11:31 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay so what do you want me to do with this one here? Ready to
merge?
Would you mind testing if having min_signal = 0.0001 or something
small
would still be okay? I am talking about the issue reported by
Rutger.
Let
me know what you think.


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Nov 1, 2014

OK - I've implemented that. While I was in there, I also implemented better
handling of data with all-zeros. This now raises a ValueError with an
informative message.

On Fri, Oct 31, 2014 at 12:28 PM, MrBago notifications@github.com wrote:

I wouldn't pull it right away just to avoid compatibility issues, None
sounds good to me though.

Bago

On Fri, Oct 31, 2014 at 7:51 AM, Ariel Rokem notifications@github.com
wrote:

That doesn't seem too bad to me, actually. I can't think of a downside
to
it, off the top of my head. Do you suggest we pull out the min_signal
kwarg altogether? Or maybe we set it per default to None?

On Thu, Oct 30, 2014 at 3:54 PM, MrBago notifications@github.com
wrote:

Just FYI, I've had issues with having the min_signal too low, but I
don't
remember exactly how to recreate it. The safest way of dealing with it
that
I've been able to come up with has been to use something like

`min_signal

data[data > 0].min()`.

On Thu, Oct 30, 2014 at 2:22 PM, Ariel Rokem notifications@github.com

wrote:

OK - I have also changed the min_signal here to deal with gh-451. Go
ahead
and merge if this looks fine to you.

On Thu, Oct 30, 2014 at 11:31 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay so what do you want me to do with this one here? Ready to
merge?
Would you mind testing if having min_signal = 0.0001 or something
small
would still be okay? I am talking about the issue reported by
Rutger.
Let
me know what you think.


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).


Reply to this email directly or view it on GitHub
#447 (comment).

assert_array_almost_equal(tensor_est.evals[0], evals, decimal=3)
assert_array_almost_equal(tensor_est.quadratic_form[0], tensor,
decimal=3)

This comment has been minimized.

@Garyfallidis

Garyfallidis Nov 1, 2014

Member

Too many spaces (pep8)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Nov 1, 2014

I 'll be happy to merge this one if you agree that it is ready @arokem.

@arokem arokem force-pushed the arokem:restore-errors branch from 189b1be to 031eef3 Nov 2, 2014

@arokem

This comment has been minimized.

Member

arokem commented Nov 2, 2014

Yes - fixed that whitespace issue in the tests, and rebased on master. Do
it!

On Sat, Nov 1, 2014 at 3:08 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

I 'll be happy to merge this one if you agree that it is ready @arokem
https://github.com/arokem.


Reply to this email directly or view it on GitHub
#447 (comment).

Garyfallidis added a commit that referenced this pull request Nov 2, 2014

Merge pull request #447 from arokem/restore-errors
BF: supposed fix for the gh-439, but still unable to reproduce OP.

@Garyfallidis Garyfallidis merged commit a3d8644 into nipy:master Nov 2, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@MrBago

This comment has been minimized.

Contributor

MrBago commented Dec 8, 2014

Sorry guys I should have been more clear and looked at this before it was merged. When I said data[data > 0].min() I meant for the whole data set, not voxel per voxel. I can see several problems with having different min_signals in different voxels.

The current implementation also has the problem of not allowing me to fit whole data sets because most data sets end up with some voxels that are all zero because of masking steps. It also breaks at least one of the examples.

@arokem

This comment has been minimized.

Member

arokem commented Dec 8, 2014

I am not doubting that at all. Could you please provide a test-case that
doesn't pass with the current code? We need that in order to start
refactoring in this direction. Obviously the tests are not sufficiently
tight as-is.

On Mon, Dec 8, 2014 at 1:44 PM, MrBago notifications@github.com wrote:

Sorry guys I should have been more clear and looked at this before it was
merged. When I said data[data > 0].min() I meant for the whole data set,
not voxel per voxel. I can see several problems with having different
min_signals in different voxels.

The current implementation also has the problem of not allowing me to fit
whole data sets because most data sets end up with some voxels that are all
zero because of masking steps. It also breaks at least one of the examples.


Reply to this email directly or view it on GitHub
#447 (comment).

@MrBago

This comment has been minimized.

Contributor

MrBago commented Dec 8, 2014

The new tests in #492 should cover the two cases that I think are most important. Let me know if you know any other cases we should test.

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