Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MiscTable.type #300

Open
hagenw opened this issue Aug 18, 2022 · 11 comments
Open

Add MiscTable.type #300

hagenw opened this issue Aug 18, 2022 · 11 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@hagenw
Copy link
Member

hagenw commented Aug 18, 2022

When looping through all tables of a database it might become easier if MiscTable would have a few more properties from Table.
For example, if you would like to gather some metadata in a single loop over all tables, and you would like to store how many filewise tables you have, you would need a check like:

if not isinstance(table, audformat.MiscTable):
    if table.is_filewise:
        count += 1

First I thought it would be good to have

  • is_filewise
  • is_segmented
    and set both to False, but I we could then have the following inconsitency:
>>> table = audformat.MiscTable(pd.Index(["f1"], name="file", dtype="string"))
>>> table.is_filewise
False
>>> audformat.is_filewise_index(table)
True

So, I would maybe only thing about adding type and setting it to 'misc'?

@hagenw hagenw added the question Further information is requested label Aug 18, 2022
@frankenjoe
Copy link
Collaborator

If you loop over db.tables you don't have to check if the table is a misc table.

@hagenw
Copy link
Member Author

hagenw commented Aug 18, 2022

If you loop over db.tables you don't have to check if the table is a misc table.

Yes, but if you a lot of other stuff in the loop and some should affect tables and misc tables it becomes cumbersome to first loop tables and afterwards loop misc tables.

@frankenjoe
Copy link
Collaborator

frankenjoe commented Jan 17, 2023

So, I would maybe only thing about adding type and setting it to 'misc'?

Ok, so the idea is that your example above can be simplified to:

if table.type != 'misc':
    if table.is_filewise:
        count += 1

?

@hagenw
Copy link
Member Author

hagenw commented Jan 18, 2023

No, it would simplify to:

if table.type == 'filewise':
    count += 1

@frankenjoe
Copy link
Collaborator

Hehe, of course :)

So yes, I agree this seems like a useful extensions.

@hagenw hagenw changed the title Add type related properties to MiscTable Add MiscTable.type Jan 18, 2023
@frankenjoe
Copy link
Collaborator

frankenjoe commented Jan 18, 2023

Ah wait, now I get it. You want to introduce 'filewise', 'segmented' and 'misc'. I thought you only want to distinguish between Table and MiscTable, i.e. 'table' and 'misc' as type. Mhh, don't we then run again into the issue that MiscTable can have a filewise index :)

@hagenw
Copy link
Member Author

hagenw commented Jan 18, 2023

I changed the title of this issue to better track what we want to have.

It's indeed not completely straightforward as before we always derived the table type from the index, compare the current documentation of Table.type:

image

My proposal would be the following for [MiscTable|Table].type:

  • for Table return filewise or segmented
  • for MiscTable return misc independent of the used index

I guess that's more complicated from a consistency/documentation point of view, but would made writing code easier.

But I'm also not 100% sure we should add this change.

@frankenjoe
Copy link
Collaborator

frankenjoe commented Jan 18, 2023

Yes, I am not sure if it is a good idea to mix up table type and index type in one field. That's why I intuitively expected that type would only have the values table and misc. This would change your example to:

if table.type == 'misc' and table.is_filewise:
    count += 1

@hagenw
Copy link
Member Author

hagenw commented Jan 18, 2023

Yes, but Table.type is already in use and returns the index type (which we set identical to table type when the misc table was not existing).

So yes, maybe we don't do anything here and propose to use code like

if isinstance(table, audformat.Table) and table.is_filewise:
    count += 1

@hagenw
Copy link
Member Author

hagenw commented Jan 18, 2023

Then you also have the option if you want to count misc tables using a filewise index as well, e.g.

if audformat.is_filewise_index(table.index):
    count += 1

@frankenjoe
Copy link
Collaborator

Agree

@hagenw hagenw added the wontfix This will not be worked on label Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants