datajoint / datajoint-matlab

Relational data pipelines for the science lab
MIT License
42 stars 39 forks source link

Difficulties clearing classes #23

Closed ahoenselaar closed 10 years ago

ahoenselaar commented 11 years ago

I started having problems when issuing clear classes:

>> hr = acq.HammerRecordings;
>> clear classes
Warning: Objects of 'acq.Events' class exist.  Cannot clear this class or any of its super-classes. 
Warning: Objects of 'acq.NeuroTimebases' class exist.  Cannot clear this class or any of its super-classes. 
Warning: Objects of 'dj.Table' class exist.  Cannot clear this class or any of its super-classes. 
>> whos
>> clear classes
Warning: Objects of 'acq.Events' class exist.  Cannot clear this class or any of its super-classes. 
Warning: Objects of 'acq.NeuroTimebases' class exist.  Cannot clear this class or any of its super-classes. 
Warning: Objects of 'dj.Table' class exist.  Cannot clear this class or any of its super-classes. 

There are no persistent variables in use and I do not remember encountering this issue a few versions back. I am concerned that we accidentally created circular references across RelVars, and Table objects. In particular, the changes to get.parents(), get.references(), get.children(), get.referencing() and get.descendants() could be responsible.

I'll bisect our commit history to figure out when this got introduced. In the meantime: Does anybody else encounter this behavior? Just bring a few BaseRelvars into your workspace, preferably ones that reference a lot of other tables and are referenced by by other tables, and then try clear classes.

aecker commented 11 years ago

I often have to alternate between "clear" and "clear classes" 2-3 times until I don't get these messages any more. It has always been like that for me. Does that work in your case?

dimitri-yatsenko commented 11 years ago

I've always had difficulties clearing classes in one go. I always chucked it to Matlab's immaturity in the OOP arena. I didn't think that this could be helped. Generally classes do not clear at all if there is an open plot that was plotted using datajoint classes. I often do close all, clear classes, clear classes.

Do you think this got worse recently?

ahoenselaar commented 11 years ago

I've tried cycles of clear and clear classes, but after 20 iterations I still get the same warnings. Older versions of DJ, going back to March 2013, have the same problem. It seems that I have never noticed this problem before.

dimitri-yatsenko commented 11 years ago

You also need to close all figures because any objects associated with a figure are not destroyed by clear classes. I have not seen a case when classes could not be destroyed after close all, clear classes, clear classes, clear classes.

ahoenselaar commented 11 years ago

There are no plots open. I start MATLAB, instantiate one object and then try clear classes. The only way to clear the classes is a restart.

dimitri-yatsenko commented 11 years ago

oh, in this case, I get no problems. Classes clear without any warnings. I have never seen this then. I only see this problem with plots, etc.

What's different between our setups?

dimitri-yatsenko commented 10 years ago

I think much of the problem comes from the use of Constant properties comprising complex objects with references to other objects. These have funny scopes in MATLAB. Perhaps MATLAB does not know how to clear these very well. Also, MATLAB makes it difficult to know what these undeleted objects are.

I am thinking that in DataJoint 2.7 we should eliminate the reliance on Constant properties. This means that the table property will not be required (but allowed for backward compatibility) and all data definition language (DDL) methods would be wrapped into dj.Relvar. Also, popRel would no longer be required to be Constant.

Do we have any evidence that the culprits are the constant properties table and popRel?

aecker commented 10 years ago

It sounds plausible. Matlab simply doesn't know in what order to clear the classes and Andreas may have a tree that is particularly ill-suited for Matlab's heuristics.

As a solution, why not just make the table a normal (non-constant) property? Looking at the constructor of dj.Table, creating a table object on the fly doesn't strike me as a particularly costly operation. I would at least try that route first to see it the table property is really the culprit. The same should apply to popRel. Creating relvars is usually fast, as long as they are not displayed. And once the properties are not constants anymore, the problem should be gone.

dimitri-yatsenko commented 10 years ago

MATLAB's suckiness is unfortunate. We may need to give up some of the elegance afforded by Constant properties.

One reason for Constant properties was to be able to address them directly without instantiating an object.

If we make popRel and table non-static properties, then patch.CleanEphys.popRel will need to become a = patch.CleanEphys; a.popRel.

With respect to table, I think I will just make dj.Relvar inherit from dj.Table. This way you will not need to add a table property to each class. You can still do it, but it's unnecessary. This means that the bare-bones Relvar class does not need to have anything in it:

%{
<<< table definition >>>
%}
classdef package.ClassName
end

I have already implemented this and it works well. I will make this release 2.7.0.

You can still add a constant table property to make it work the same way as before. But now you can apply the data-definition methods directly to the dj.Relvar object rather than to its table property.

drop(package.ClassName)
alterAttribute(package.ClassName, '...')

Again, none of this breaks backward compatibility. This release will fix issues 22, 23, 24.

dimitri-yatsenko commented 10 years ago

Pushed the fix on master: f4f9b2cf9607c7d76a41b9889e7cec710f40587a. Closing.

ahoenselaar commented 10 years ago

With table gone, there seem to be fewer issues regarding the clearing of classes. But popRel is still a constant property for all auto-populated tables and is causing the same problems with the garbage collector.

dimitri-yatsenko commented 10 years ago

popRel is no longer required to be Constant. It is allowed to be constant for backward compatibility. When you create a new class using dj.new, popRel will be declared as a regular property.

ahoenselaar commented 10 years ago

Ah, thank you for the clarification! That should solve our remaining issues.