Skip to content
This repository has been archived by the owner on Dec 11, 2023. It is now read-only.

issue regarding deletion of moved columns #170

Merged
merged 2 commits into from
Apr 19, 2015
Merged

issue regarding deletion of moved columns #170

merged 2 commits into from
Apr 19, 2015

Conversation

esc
Copy link
Member

@esc esc commented Mar 29, 2015

As reported in #137 there is an issue with removing on-disk columns that had been created previously elsewhere:

http://nbviewer.ipython.org/gist/alimanfoo/6cc03872fe8054e63860/bcolz_ctable_delcol.ipynb

Basically, the problem is that when moving a column into the ctable, the rootdir of that carray isn't being updated.

@esc esc added the bug label Mar 28, 2015
@esc esc added this to the v0.9.0 milestone Mar 28, 2015
@FrancescAlted
Copy link
Member

I think a fix for this should be in 0.9 yes.

@esc
Copy link
Member Author

esc commented Mar 29, 2015

I attached a pull-request to this issue which should fix the erroneous behavior in question. @alimanfoo maybe you could confirm it solves the issue.

@alimanfoo
Copy link
Contributor

Thanks @esc, this works for me, i.e., the rootdir for the originally-on-disk column is removed after calling delcol, notebook rerun with this PR.

FWIW I think it would be better to make a shallow copy (view?) of the originally-on-disk carray object passed in to addcol before modifying the rootdir attribute. As it currently stands, passing an on-disk carray into addcol has the side-effect of modifying the original carray object's rootdir, which is potentially confusing if the user still wants to use the original carray object in any way, because they would (like me) probably expect it to still have the original rootdir it was created with. Make sense?

@esc
Copy link
Member Author

esc commented Mar 30, 2015

Yes, I fear that this feature and how it should work exactly will warrant some more in-depth discussions.

@esc
Copy link
Member Author

esc commented Apr 1, 2015

I've taken it to the mailinglist.

@esc
Copy link
Member Author

esc commented Apr 11, 2015

The mailinglist was fairly quiet so I have implemented a full copy of the column. I think this is a) what users would expect and b) delegates the internals to the carray itself. Note however, that when adding an in-memory column the python object is not copied, which may cause confusion yet again.

In addition, this feature isn't fully cooked yet so this is to be considered as more of an interim bug-fix and the API will likely undergo some more refactoring.

@alimanfoo
Copy link
Contributor

FWIW as a user I'm comfortable with this solution.

On Saturday, 11 April 2015, Valentin Haenel notifications@github.com
wrote:

The mailinglist was fairly quiet so I have implemented a full copy of the
column. I think this is a) what users would expect and b) delegates the
internals to the carray itself. Note however, that when adding an in-memory
column the python object is not copied, which may cause confusion yet again.

In addition, this feature isn't fully cooked yet so this is to be
considered as more of an interim bug-fix and the API will likely undergo
some more refactoring.

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

Alistair Miles
Head of Epidemiological Informatics
Centre for Genomics and Global Health http://cggh.org
The Wellcome Trust Centre for Human Genetics
Roosevelt Drive
Oxford
OX3 7BN
United Kingdom
Web: http://purl.org/net/aliman
Email: alimanfoo@gmail.com
Tel: +44 (0)1865 287721

@esc
Copy link
Member Author

esc commented Apr 19, 2015

Ok, it's decided, we are taking it.

@esc
Copy link
Member Author

esc commented Apr 19, 2015

Aight, here goes.

esc added a commit that referenced this pull request Apr 19, 2015
issue regarding deletion of moved columns
@esc esc merged commit ac52154 into Blosc:master Apr 19, 2015
@esc esc deleted the vh/fix/copy_carray branch April 19, 2015 12:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants