[FIX] Table.add_column - Do not try to insert data in locked empty array

[PrimozGodec]

Issue

When a table has no rows add_column still tries to insert (empty) data to the table's array. In case the table's array is empty Orange doesn't unlock it and so inserting fails.

Description of changes

Skip inserting empty data when the array is empty

Includes

• Code changes
• Tests
• Documentation

[PrimozGodec]

Here is the PR that introduced skipping locking for empty tables #5714

This avoids numpy's deprecation of making arrays with .base == None that
do not own their data writable. This strange combination appears when
unplickling arrays of size 0.

[janezd]

But it seems that @markotoplak also fixed unlocking for size == 0. Your tests pass even without your change in add_column. I'd keep the tests, but remove the change in add_column - unless there's another scenario that requires it.

[PrimozGodec]

But it seems that @markotoplak also fixed unlocking for size == 0.

If I understand correctly, @markotoplak's fix just skips unlocking the table in this case (here: https://github.com/markotoplak/orange3/blob/cd5d384da7a34a2f0f57de0eccee2a2ca83d4de6/Orange/data/table.py#L508-L510).

That is the part of the comment from his PR:

For now, numpy allows unlocking (setting writable = True) for these arrays but shows a deprecation warning. To avoid the warning, the last commit disables unlocking of size=0 tables.

Your tests pass even without your change in add_column. I'd keep the tests but remove the change in add_column - unless there's another scenario that requires it.

On my computer (when setting Table.LOCKING=True), the test fails without a change in the add_column. The underlying array is locked, and I get ValueError: assignment destination is read-only. Did you make sure that Table.LOCKING = True for you? We either need to do this check in add_column or change unlocking, but then we reintroduce the problem that @markotoplak fixed.

[janezd]

You're right, I forgot to enable Table.LOCKIING. I forgot we do it only for tests.

add_column calls set_column and the exception is raised there. Wouldn't this problem also occur when one calls set_column? Perhaps we should rather change set_column, like this

        # Zero-sized arrays cannot be made writeable, yet the below
        # assignment would fail despite doing nothing.
        if len(self) > 0:
            self._get_column_view(index)[:] = data
        else:
            assert len(self) == len(data)

or assert len(data) == 0.

Even if we change add_column, I think we should assert that the length of the data is correct, otherwise assigning a non-empty column to an empty table will fail silently.

[PrimozGodec]

I agree it can be moved there, and checking the size is also a good idea.

Initially, I put it inadd_column since unlocking happened there, but it is better to have this check in set_column.

[janezd]

... because unlocking is not a problem, so let us not skip it. Unlocking does not raise an error (it just fails to do anything); setting the data does.