replace the original_directory path in the low-level db too

Since some low-level dbs (most of them) accept original_directory and annotation_directory, it is best to replace the paths there too. Here I am assuming that the low-level db exists in BioDatabase._db and have made sure this is the case for all high-level databases. This may not be the solution so I am open to suggestions.

enabled an automatic merge when the pipeline for bf0ef461 succeeds

mentioned in commit 31d905b0

merged

This is an important detail to make sure your high-level implementations are working @bob

Hmm... I see what you are doing, but I don't really like this. It assumes that the low-level database is stored in the _db attribute, and that the low-level database actually uses the _db.original_directory and _db.annotation_directory attributes. If this is not the case, your solution won't work -- at least it would not not harm.

What do you think about a function update_directories(original_directory, annotation_directory) with a default implementation as you have (using _db), but which can be overloaded in high-level databases on need? In this case, please do not swallow the AttributeError in https://gitlab.idiap.ch/bob/bob.bio.base/blob/bf0ef461b9f55dca486d98c398aa61eee48d7c97/bob/bio/base/database/database.py#L200 and https://gitlab.idiap.ch/bob/bob.bio.base/blob/bf0ef461b9f55dca486d98c398aa61eee48d7c97/bob/bio/base/database/database.py#L208, but raise an appropriate exception (for example saying: "The high-level database does not contain the low-level database as _db; please overload the update_directories function in the high-level database".

@mguenther I didn't like this at all but it was a temporary solution to make sure databases are working.

What do you think about a function update_directories(original_directory, annotation_directory) with a default implementation as you have (using _db), but which can be overloaded in high-level databases on need? In this case, please do not swallow the AttributeError in https://gitlab.idiap.ch/bob/bob.bio.base/blob/bf0ef461b9f55dca486d98c398aa61eee48d7c97/bob/bio/base/database/database.py#L200 and https://gitlab.idiap.ch/bob/bob.bio.base/blob/bf0ef461b9f55dca486d98c398aa61eee48d7c97/bob/bio/base/database/database.py#L208, but raise an appropriate exception (for example saying: "The high-level database does not contain the low-level database as _db; please overload the update_directories function in the high-level database".

I think that would be a proper solution.

mentioned in merge request bob.bio.spear!30 (closed)

@mguenther I have been thinking about this and I think we should drop this! Instead, I think this should be handled by the high-level interface since only that object knows about the existence of a low-level object. I think this can be implemented using Python's class property : https://docs.python.org/3/library/functions.html#property

What do you think? This has to be implemented in all high-level implementations in bob.bio.face, bob.bio.spear, bob.bio.video, bob.pad.face, and bob.pad.voice though so I would appreciate everyone's feedback. @bob

The final solution would be something like:

from bob.bio.base.database import BioDatabase
class AtntBioDatabase(BioDatabase):
    def __init__(
            self,
            name='atnt',
            original_directory=None,
            original_extension='.pgm',
            **kwargs
    ):
        super(AtntBioDatabase, self).__init__(
            name=name,
            original_directory=original_directory,
            original_extension=original_extension,
            **kwargs)

        from bob.db.atnt.query import Database as LowLevelDatabase
        # notice how original_directory and original_extension is given to the
        # low level db if it accepts it
        self._db = LowLevelDatabase(original_directory, original_extension)

    @property
    def original_directory(self):
        return self._db.original_directory

    @original_directory.setter
    def original_directory(self, value):
        self._db.original_directory = value

mentioned in commit 683f9968

mentioned in merge request !101 (merged)

I like the idea of having the a @property attribute. What do you think of having a default implementation (which raises an exception if it does not work) in the base class? I can provide an implementation for that in !101 (merged)

Hmm... This does not seem to work. I get an AttributeError.

Here is what is causing the issue. When creating a database, such as in https://gitlab.idiap.ch/bob/bob.bio.base/blob/master/bob/bio/base/test/dummy/database.py I first call the base class constructor, which in turn tries to set the property (indirectly through the bob.db.base.Database constructor): https://gitlab.idiap.ch/bob/bob.bio.base/blob/master/bob/bio/base/database/database.py#L92

The problem is that self._db is initialized after calling the constructor: https://gitlab.idiap.ch/bob/bob.bio.base/blob/master/bob/bio/base/test/dummy/database.py#L19 Hence, during constructor call, I get the following error: 'DummyDatabase' object has no attribute '_db'

This issue would be persistent, no matter where we implement the property, either in the base class, or in each derived class.

One solution would be to create the self._db before calling the base class constructor. However, this would have to be done for all high-level databases, including any newly generated database (possibly be someone external to Idiap). I can see a lot of trouble there.

Any other ideas?

What do you think of having a default implementation (which raises an exception if it does not work) in the base class? I can provide an implementation for that in !101 (merged)

This is bad! It will still assume some about structure about dbs while it should not know about them. High-level db interface's codes may look similar but they are different implementations. As I said before, it is up to the high level db to implement the API correctly.

We can fix this in all high-level implementations at once.

We had this discussion before. I like to avoid copying code as much as possible. Bug fixes in the base class are easier and reflect in all other derived classes, while bug fixes (or added functionality) in derived classes need to be implemented one by one.

I agree that my current solution is not good, while my fix would make it more flexible while still not needing to touch most of the classes. However, the issue with the order of self._db and the constructor still applies.

We had this discussion before. I like to avoid copying code as much as possible. Bug fixes in the base class are easier and reflect in all other derived classes, while bug fixes (or added functionality) in derived classes need to be implemented one by one.

I know it is easier but they are conceptually different things. For example, there is no self._db in FileListBioDatabase.

However, the issue with the order of self._db and the constructor still applies.

Fixing it in all high level interfaces one by one fixes this too.

That is true. The question is: should we go for that way of solving the issue? Who is documenting this new feature?

Actually: I don't remember, why I had implemented that hack in the first place. Maybe we can live without it. As all of our high-level databases take care of the directories of original data and annotations, why do we need to change the original directory of the low-level database?

why do we need to change the original directory of the low-level database?

It will fail when annotation is called:

class HighLevelDB:
  def annotation(self, file):
    return self._db.annotation(file._f)

See https://gitlab.idiap.ch/bob/bob.bio.face/blob/d6f75c9aaa58dc4fb6968a47aecced79b048419a/bob/bio/face/database/mobio.py#L71 and https://gitlab.idiap.ch/bob/bob.db.mobio/blob/0a544602f98f99040aaa428a716a120e681d4c9c/bob/db/mobio/query.py#L641 for an example.

Oh, indeed, there the annotation_directory is used in the low-level database. I am not sure if the original_directory is used anywhere, but since we have to implement something, why don't you go ahead and change the high-level database implementations to use the property functions, where one of the two directories is actually used?