WIP: Resolve "Some database interfaces do not provide access to the annotations stored in the low-level databases"

Added 2 commits:

7a166e6c - Corrected usage of file in database
0b1ae0cc - Corrected interface for arface

Added 1 commit:

f47b1a2f - Updated documentation of already handled databases

This looks OK to me, but will let @tiago.pereira or @amohammadi comment on it as they are the most affected parties around here.

This does not look OK to me. We worked so hard to remove these layers, layers, and layers of classes and now you just added two more. I believe as I said before it is the job of the implementation to handle things. A proper solution IMHO is to do this for each database without creating helper classes. This will also help people realize what they are doing when creating a high-level implementation. If you still want to push this approach, I think it is best that you re-open this issue: bob.bio.base#40 (closed) which will ease down things here.

I think this question with the annotationsmethod in the low level is similar (if not the same) with the load method (discussed in bob.bio.base#40 (closed)). In the end wasn't a good idea to "kill" the low level object in the high level constructor. In some corner cases we are already keeping it via aggregation (NIST SRE, bob.db.verafinger, bob.db.utfvp, some heterogeneous face databases). The list tends to grow.

I think we don't need the FaceBioFileWithAnnotations and the FaceBioDatabaseWithAnnotations classes @mguenther. We can make the low level file required by default in the constructor of FaceBioFile, VeinBioFile and SpeakerBioFIle (aggregation). This would solve the corner cases with the load of raw data (in case of low level has some special load function) and will solve the load of annotations. For instance, this could be perfectly FaceBioFile the FaceBioDatabse https://gitlab.idiap.ch/bob/bob.bio.face/blob/14-some-database-interfaces-do-not-provide-access-to-the-annotations-stored-in-the-low-level-databases/bob/bio/face/database/database.py#L17 Would solve the both cases of annotations that we have (annotations in file lists and annotations in the sqlite)

This would eliminate extra layers and will make the things simpler too.

I think we should discuss this during the coffee so we don't "cook" this issue too much.

@amohammadi: There is a strict programming paradigm that I follow and that says: Avoid copying code. If everyone would implement their own way of handling annotations that are stored in the database, everyone would need to write exactly the same code. Hence, an intermediate class looks fine to me.

@tiago.pereira: I agree with you, though I am afraid that forcing to have a bob.db.base.File to be wrapped might hinder people to write their databases. For example, the new implementation of the FileList database (as indicated in bob.bio.base#52 (closed)) might not use the bob.db.base.File at all -- I haven't seen @pkorshunov's implementation yet. Hence, we would need at least two layers of databases, one as it currently is in bob.bio.base.database.BioDatabase and one derived class similar to FaceBioDatabaseWithAnnotations (which we should rename and move to bob.bio.base).

Finally, this would basically go back to my previous implementation -- somewhat at least. There I also had a Database, a DatabaseBob and a DatabaseFilelist (see bob.bio.base#52 (closed)). Maybe my initial design was not that bad at the end? ^^

I know it's copying code but at least people would understand our database designs better instead of everything magically working for them. Right now, explaining to people that they need to have a low level and a high level interface is confusing enough and telling them another layer exists that helps if you have annotations is just going to be more confusing. When they sit down and want to create a new database, they should know by looking at bob.bio.base.database.BioDatabase how to handle annotations.

I also agree forcing of having a low-level file is not a good idea.

changed milestone to %Refactoring 2016 and gitlab migration milestone

@mguenther do you mind if I go ahead and implement this in a code copying way that I mentioned? If you mind it, we can merge this as is since things are broken for months and we need to fix them.

You can look at !15 (merged) to see how to handle annotations while writing the high level interface.

mentioned in merge request bob.bio.base!56 (merged)

@amohammadi OK with me.

closed

WIP: Resolve "Some database interfaces do not provide access to the annotations stored in the low-level databases"

Merge request reports

Activity