g-node / nix Goto Github PK

Since datasets (in addition to groups) also can have attributes maybe we should have a common sub-object (BaseObject?) or move the attribute code to an different utility class entirely? Or is this not needed in the current draft of the standard?

Methods for group / object handling: open, delete, create.

Currently all our 'front facing' objects, e.g. Block, File, etc pp. that internally work with a shared pointer (shared_ptr) don't check if the shared_ptr is valid (is not having a nullptr internally). A shared_ptr being null can happen 'legally' because we have a default constructor on all these objects and we allow nullptr assignments. The current standard libraries, i.e. libc++ (from clang/llvm 3.3) and libstdc++ (from gcc 4.8), don't seem to have a check in place either (cf. https://gist.github.com/gicmo/8958152), so dereferencing the shared_ptr will just dereference the shared pointer itself, which is undefined behaviour (and most likely a crash).

The question remains, should we throw an exception if someone calls a method on a front-facing object which has an invalid/unassigned shared_ptr or do nothing (like nil in Objective-C)?
Additionally we need a way to check if an object is valid or not, by having a isValid method or implementing explicit bool operator() const; conversion operator (or both).

Implement a virtual swap method an use it to in operator= in PandoraEntity and all its subclasses.

First I think there is an error: upon creation of a sub-source in the SourceHDF5::create_source there is no parent being set. (As it is being done in SectionHDF5::create_section).

Second this brings up in general the issue in how far the fact that an entity (source / section) is created as sub-entity in a class of the same type is equivalent of that class being its' parent.
It is logical to assume so, but as other implementations might be done by other people, they might commit the exact same error as we have regarding not setting a class as parent. Therefore we could think of removing the constructor without "parent" parameter, thus always requiring a parent to be passed. Then the constructor code would check what was passed as parent and if it is not a legit parent (e.g. of same class type) then it will not set the parent. This could be achieved using "dynamic_cast".

So far, the naming of methods is not consistent among classes:

1. remove, delete methods
   general rule: if 1-to-n relation that is realized in nested structures, it should be createXXX and deleteXXX. Relations that are modeled with via references to the entity_id. (For example a DataTag stores the positions and the extents as references to DataArrays. Then it should be removePositions() which simply removes the reference but does delete the referenced DataArray.

As spotted by clang:

./nix/include/nix/hdf5/EntityWithSourcesHDF5.hpp:72:10: warning: 'nix::hdf5::EntityWithSourcesHDF5::swap' hides overloaded virtual function
      [-Woverloaded-virtual]
    void swap(EntityWithSourcesHDF5 &other);
         ^
.//nix/include/nix/hdf5/EntityHDF5.hpp:65:18: note: hidden overloaded virtual function 'nix::hdf5::EntityHDF5::swap' declared here
    virtual void swap(EntityHDF5 &other);

Implement all section related methods in the class EntityWithMetadata using the ReferenceList helper class (see EntityWithSources)

The model defines some 1-n or n-m relations between entities. A File may have many Blocks contain Blocks, a Block may contain Sources, Tags, DataArrays and Representations. To ease the handling of those related objects the parent object provides a method that creates an iterators.

Currently there is no type of class defined for filter methods which makes method signatures very verbose:

findSections(std::function<bool(const Section&)> filter = util::acceptAllFilter<Section>, ...);

It is still used for the non-functional Value stuff.

it should be considered to write more STL style code in the base and implementations, e.g.:

for (typename std::vector<T>::iterator it = tags.begin(); it!=tags.end(); ++it)

instead of

for (int i = 0; i< tags.getCount(); i++)

Otherwise the higher level code has to interact with old school type iteration and this leads to the necessity of having e.g. get-functions that accept indices:

SimpleTag BlockHDF5::getSimpleTag(const string &id) const 
SimpleTag BlockHDF5::getSimpleTag(size_t index) const

So the higher level programmer can do:
for (typename std::vector<T>::iterator it = tags.begin(); it!=tags.end(); ++it) {
    *it = getSimpleTag( i++ );
}

[gicmo: edited for formatting]

include/pandora/Property.hpp:127:15: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
    if (index < 0 || index >= valueCount()) {
        ~~~~~ ^ ~

and

src/TreeIterator.cpp:9:13: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
  if (depth < 0)
      ~~~~~ ^ ~

Investigate what is needed to compile nix on windows.

With the commit dadb04f the main.cpp was removed. This leads to following error:
clang: error: no such file or directory: 'main.cpp'
Either we add main.cpp again or we get rid of the target in the Makefile.

We currently use HDF5 attributes for staring created_at and updated_at. HDF5 already provides a mechanism for recording such information automatically via H5Oget_info[1]. That times will be tracked must be ensured via the corresponding property set by H5Pset_obj_track_times.

[1] http://www.hdfgroup.org/HDF5/doc/RM/RM_H5O.html#Object-GetInfo
[2] http://www.hdfgroup.org/HDF5/doc/RM/RM_H5P.html#Property-SetObjTrackTimes

We need to fix the speed issues

Maybe it would be good to have a little document where we 'codify' the style of coding within the project, like other projects do: [1],[2],[3]. It doesn't have to be that exhaustive but basic indention style, git commit messages, method naming, etc. pp. would be good. It would surely also help for people new to the project.

[1] http://clang.llvm.org/hacking.html
[2] http://developer.gimp.org/HACKING
[3] https://git.gnome.org/browse/gvfs/tree/README.commits

Currently the DataArray::getData and DataArray::setData functions are working on all of the data. We need at least a way to specify offset (and size for read).

In the current API some of the methods called removeXXX not only remove an association but physically delete an entity from the file/backend. Here #51 it was discussed, that those methods should be named deleteXXX.

Here is a list of methods that should be renamed. Check all the methods in the list if they really delete something and rename them:

bool File::removeSection(const string&)
bool File::removeBlock(const string&)

bool Section::removeProperty(const string&)
bool Section::removeSection(const string&)

void Property::removeValues()

bool Block::removeSource(const string&)
bool Block::removeDataArray(const string&)
bool Block::removeSimpleTag(const string&)
bool Block::removeDataTag(const string&)

bool DataArray::deleteDimension(size_t id)

bool DataTag::removeRepresentation(const string&)
bool SimpleTag::removeRepresentation(const string&)

Remove the link type untagged.

Make the tests go green, green, green!

Implementation of a method that allows the user to find related sections by various criteria. The signature may look like this:

Section::findRelated(function<bool(const Section&)> filter) const;

With filter having a default that matches all related sections.

Implement the class DataTag.

Since pandora is already used by another neuroscience related software project, we have to find a new name.
Namespaces, include guards and other occurrences of pandora have to be replaced with the new name.

HDF5 itself records mtime, ctime, etc. It can be accessed via the H5Oget_info() API. Currently we record created_at and updated_at ourselves via EntityHDF5.

BaseContainer should provide methods for reading and writing attributes with different types and a method that checks the existence of attributes. List of needed getters for attributes (we'll need of course also the setters):

getAttr(string name, string &value)
getAttr(string name, double &value)
getAttr(string name, long long &value)

Other methods for attributes:

hasAttr(string name)
delAttr(string name)

We need also some functions for datasets that act as attributes.

getData(string name, vector<double> &values)
getData(string name, vector<vector<double>> &values)

getData(string name, vector<string> &values)
getData(string name, vector<vector<string>> &values)

Other methods for Datasets:

hasData(string name)
delData(string name)
openData(string name)

Methods for Groups or Objects:

hasGroup(string name)
delGroup(string name)
openGroup(string name, boolean create = true)

Currently properties of entities that are optional (see model) are not handled very nicely. Some examples:

// returns "" if not set
string NamedEntityHDF5::definition() const;

// you need to pass "" if you want to remove the definition
void NamedEntityHDF5::definition(const string&) const;

// unable to pass nullptr or NULL
void DataTag::extents(const DataArray &pos)

Solution for setter

If the type of the property is derived from ImplContainer e.g. Section it is sufficient to add a constructor that gets one parameter from the type nullptr_t and handle the case where the parameter is null correctly in the setter method.

If the type of the property is not an ImplContainer but a string or some other type like definition in the example above this can be solved through polymorphism:

void NamedEntityHDF5::definition(nullptr_t) const;
void NamedEntityHDF5::definition(const string&) const;

Solution for getter

If the type of the property is an ImplContainer the getter can just return an empty container that evaluates to true when compared with nullptr_t.

In other cases it might be a good idea to use boost optional.

Currently no method exists that allows the user to explicitly close files. However, since the the file is physically closed in the dtor of FileHDF5, files are closed when the last reference is deleted:

File f = File::open("fname.h5");
f = NULL;    // file is closed implicitly

File f1 = File::open("fname.h5");
File f2 = f1;
f1 = NULL;  // file is not closed because f2 still 
            // holds a reference

It would be nicer to have a close method on File and IFile and maybe a method isOpen():

File f1 = File::open("fname.h5");
File f2 = f1;
f1.close();     // closes the file that is referenced 
                //by f1 and f2
f2.isOpen();    // --> false

Investigate the warning and see if it is possible to fix it.

We need a good way of error and exception handling

Replace the home-grown Makefile by the CMake build system.

Since we now have switched the pull-request dev model I think it would be nice to have the main nix/pandora repo in a cleaned of outdated branches and in a state where we only have branches at this repo that are interesting to the 'common public', i.e. people interested in using (or helping develop) nix/pandora (the old_stable is such an example, but I think everything else should go).

Are there any objections to that?

Implement the three dimension classes. The order property should be used as id for all dimension entities.

Unit conversion to convert positions and extents of Tags to real values and access the data.

Virtual dtors in IFoo classes are missing and thus dtor chain is not given and classes would leak. Would since they actually might not, since they are maintained by a shared pointer which takes care of calling dtors (it saves a deleter for each class).

But the safer solution is to have virtual dtors in IFoo.

Currently all source code for tests is actually included in the header files for each test and the cpp files are clones of each other. This code structure is not nice. We should move all implementation to the cpp files again and run all tests through a UnitTestRunner binary, which either runs all tests or only the one specified on the command line. The latter feature is already in TestAll.

Currently DataArray only supports reading and writing of all the data. Additionally we need a streaming interface.

Create the following unit tests and make sure that they are complete:

• TestFile
• TestBlock
• TestDataArray
• TestSimpleTag
• TestDataTag
• TestRepresentation
• TestSource
• TestSection
• TestProperty
• TestValue
• TestDimension

Forgot to include <functional> for std::function.

In file included from /Users/gicmo/Coding/G-Node/nix/src/Dimensions.cpp:10:
In file included from /Users/gicmo/Coding/G-Node/nix/include/nix/Dimensions.hpp:12:
/Users/gicmo/Coding/G-Node/nix/include/nix/base/ImplContainer.hpp:51:14: error: no type named 'function' in namespace 'std'
        std::function<bool(TENT)> filter) const
        ~~~~~^
/Users/gicmo/Coding/G-Node/nix/include/nix/base/ImplContainer.hpp:51:22: error: expected ')'
        std::function<bool(TENT)> filter) const

Use const std::string &s instead of std::string s for parameters.

We need to update the example files and images. According to the rule 1-n connections should be modeled as trees, while n-m connections are represented in the flat layout...