Skip to content

Conversation

@choldgraf
Copy link
Contributor

I believe this fixes #4514 and adds a test for it...should double check the tests @larsoner since I wrote this while on a flight back from NYC and my brain was a little fried :-)

rf = ReceptiveField(tmin, tmax, 1, feature_names, fill_mean=True,
estimator=mod)
rf.fit(X, y)
assert_array_equal(rf.delays_, np.arange(tmin, tmax + 1))
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test that fill_value actually does anything. Can you add a test that fails in master but passes here?

Copy link
Contributor

Choose a reason for hiding this comment

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

more over, fill_value is not in ReceptiveField's signature here

Copy link
Member

Choose a reason for hiding this comment

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

fill_value isn't really meant to be public

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is not referring to ReceptiveField's signature?
img

Copy link
Member

Choose a reason for hiding this comment

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

Yeah you're right, didn't see it was in __init__ -- it's also not supposed to be publicly exposed, so this is the wrong way to check it

@larsoner
Copy link
Member

larsoner commented Sep 8, 2017

@choldgraf do you want me to take over?

@choldgraf
Copy link
Contributor Author

@larsoner if you've got the time, please go for it...been at neurohackweek for the last week and my to-do list has exploded as a result :-P

@larsoner
Copy link
Member

larsoner commented Sep 8, 2017

Closing for #4543

@larsoner larsoner closed this Sep 8, 2017
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.

Issue w/ fill_mean in ReceptiveField

3 participants