Skip to content

[WIP] Fix read only mmap tests #5507

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.

Sign up for GitHub

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

Jump to bottom
Closed
pletelli wants to merge 5 commits into from
Closed

[WIP] Fix read only mmap tests #5507

pletelli wants to merge 5 commits into from

Conversation

@pletelli
Copy link

@pletelli pletelli commented Oct 21, 2015

will soon fix #5481 which has been opened to fix tests on #4807

This PR is based on #4807

@pletelli pletelli force-pushed the fix_read_only_mmap_tests branch from 47bea48 to 0d4ef7e Compare October 21, 2015 10:14
ogrisel
ogrisel reviewed Oct 21, 2015
View reviewed changes
sklearn/utils/validation.py
Copy link
Member

@ogrisel ogrisel Oct 21, 2015

Choose a reason for hiding this comment

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

The class is numpy.ndarray, never numpy.array.

@amueller
Copy link
Member

amueller commented Oct 21, 2015

Also see #4807 ?

pletelli
pletelli reviewed Oct 21, 2015
View reviewed changes
sklearn/utils/validation.py Outdated
Copy link
Author

@pletelli pletelli Oct 21, 2015

Choose a reason for hiding this comment

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

this appears not to be the right place to raise that error.
check_array() can be called in both fit() and transform() as in RobustScaler for example.

The returned array is modified inplace in transform() but no modification is ever performed in fit().

Could we make this check at the beginning of each transform() method ? WDYT @ogrisel
This may be in the mother class's method fit_transform() and used calling super()

@pletelli pletelli force-pushed the fix_read_only_mmap_tests branch 6 times, most recently from 06ba2a8 to 5bfb9f7 Compare October 21, 2015 15:57
@pletelli pletelli force-pushed the fix_read_only_mmap_tests branch from 5bfb9f7 to d291d89 Compare October 21, 2015 15:58
arthurmensch
arthurmensch reviewed Oct 21, 2015
View reviewed changes
sklearn/cross_decomposition/pls_.py
Copy link
Contributor

@arthurmensch arthurmensch Oct 21, 2015

Choose a reason for hiding this comment

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

this should maybe factored into check_array:
Parameters copy=False|True|'on_readonly'

@arthurmensch arthurmensch mentioned this pull request Oct 21, 2015
Closed
1 task
@arthurmensch
Copy link
Contributor

arthurmensch commented Dec 7, 2015

@pletelli are you still willing to work on this or should I take up ?

@Morikko Morikko mentioned this pull request Mar 12, 2017
Closed
@lesteve lesteve mentioned this pull request Feb 20, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Estimators should not try to modify X and y inplace in order to handle readonly memory maps

5 participants

@pletelli @amueller @arthurmensch @ogrisel @GaelVaroquaux