Should be stored

I actually think these try/excepts are no longer necessary and only add little extra information for the user. Could you please remove them?

Is there a reason to define these arguments outside of the function you'll be calling?

I found it more didactic to have a comment and an assignment instead of just assigning inside the function. WDYT?

The current example has comments inside the function so everything is in one place. Not sure which one is better though as the current version has a pretty long function call.

I discussed this with @ArlindKadra regarding #547 to have this based on the length. But that would distribute the creation of the arguments. I start to see why creating them beforehand is a good idea.

In the long run we will probably make pandas required (it is already required for the examples and documentation), so I'd totally okay if you add it as a dependency.

This could be deleted now, right?

Could you please download and check that everything was converted and uploaded properly?

I assumed that I can do that with something like:

upload_id = dataset.publish()
dataset_uploaded = openml.datasets.get_dataset(upload_id)

However, I receive the following exception:

openml.exceptions.OpenMLServerException: https://test.openml.org/api/v1/xml/data/features/5915 returned code 273: Dataset not processed yet

What is the usual way that you are doing it.

One way could be something like that:

        upload_id = dataset.publish()
        while True:
            try:
                dataset_uploaded = openml.datasets.get_dataset(upload_id)
                break
            except OpenMLServerException as msg_exception:
                if "code 273" in str(msg_exception):
                    pass
                else:
                    raise msg_exception

but I am not sure how long it can take for the dataset to be processed and we could get in some timeout nightmare.

@glemaitre We already had this problem in #547. We decided to only compare based on the arff and we also implemented 2 functions which get the arff and format based on the id (those are available when it is uploaded). I have to refactor my pr a bit more, but it should be available tomorrow.

So do you mean to use this function:

https://github.com/openml/openml-python/pull/547/files#diff-2ffad3a1ae43bbcc0e841e2f1df8967bR692

So I would probably wait that your PR get first in and I will manage the conflict then.

yes, @mfeurer are you okay with this?

This looks good at first glance.

Usually we want this to be category as well, right? kinda tricky ;)
And what about integers that encode categories?

And what about integers that encode categories?

If they encode categories, then one should use the category dtype for those columns.

However, this is part needs to be modified. We should manage INTEGER and REAL/NUMERIC differently. We also have to think more in depth about the missing values. I'll use the expertise of @jorisvandenbossche

Basically, we are working on uploading the datasets from dirty_cat and we are confronted to those issues.

Yes, I would also say that it is up to the user to use a categorical dtype. Actual string columns are not that rare (eg titanic dataset), and it is quite impossible to guess from a string column if it should be categorical or not.

Of course, it could also be a keyword that the user can pass to indicate which columns to see as categorical without the need that the (s)he does the actual conversion themselves.

And what about integers that encode categories?

I think arff does not support integer categories?

Conceptually, arff should allow this (as long as the categories are integers, too). However, liac-arff does not support this. Can be reproduced with:

import arff

arff.dumps({
    'relation': 'test',
    'description': 'abc',
    'attributes': [['test', [1, 2, 3]]],
    'data': [[1], [2]],
})

I'm wondering if it's worth changing this. Formally, there's also no difference between REAL, INTEGER and NUMERIC, but I don't see a reason to not be more specific about the data type as the arff specification allows this.

I think that there is a difference between REAL and INTEGER since the dtype will be kept, isn't it.

In [5]: X = arff.dumps({ 
   ...:   'relation': 'test', 
   ...:   'description': 'abc', 
   ...:   'attributes': [['test', 'REAL']], 
   ...:   'data': [[1], [2]] 
   ...: })                                                                                              

In [6]: arff.loads(X)                                                                                   
Out[6]: 
{'description': 'abc',
 'relation': 'test',
 'attributes': [('test', 'REAL')],
 'data': [[1.0], [2.0]]}

In [7]: X = arff.dumps({ 
   ...:   'relation': 'test', 
   ...:   'description': 'abc', 
   ...:   'attributes': [['test', 'INTEGER']], 
   ...:   'data': [[1], [2]] 
   ...: })                                                                                              

In [8]: arff.loads(X)                                                                                   
Out[8]: 
{'description': 'abc',
 'relation': 'test',
 'attributes': [('test', 'INTEGER')],
 'data': [[1], [2]]}

Going by the official arff specification, all numeric attributes should go by NUMERIC
https://www.cs.waikato.ac.nz/ml/weka/arff.html

In order to make OpenML datasets comply with as much as possible arff parsers, maybe it's best to enforce this too server side? In that case the Python uploader should of course also upload NUMERIC key. @mfeurer @joaquinvanschoren @berndbischl WDYT?

Have to add that currently this is not enforced, server side

@janvanrijn you pointed to an outdated version of the arff standard (see the first sentence on the page you linked to), the newer one (https://waikato.github.io/weka-wiki/arff_stable/) actually mentions 'integer' and 'real' (although it says they are treated as numeric)

I think they're more a guideline than a necessity. But it woudl be great if we can support them as good as possible in pandas. I don't think this would be easy in numpy, though.

you don't necessarily need to pass the dtype here, you could also iterate only through the column names

Yes, I would also say that it is up to the user to use a categorical dtype. Actual string columns are not that rare (eg titanic dataset), and it is quite impossible to guess from a string column if it should be categorical or not.

Of course, it could also be a keyword that the user can pass to indicate which columns to see as categorical without the need that the (s)he does the actual conversion themselves.

And what about integers that encode categories?

I think arff does not support integer categories?

Doesn't this do the exact opposite? It overrides the attributes from the dataframe with the arguments passed by the user, right?

Yes. It is the mechanism that allow the user to overwrite some specific attribute. So it could be useful to force a specific data type or specify the categories (e.g. if there is some missing categories in the data column).

In one of your comments you mentioned that this needs to be at least 0.19. Would it make sense to add this piece of information here?

This could be deleted now, right?

You might need to allow 'unicode' here as well for python 2 compat ?

interger -> integer

The doc string that you applied seems different from the other docstrings that we use, i.e.,

Parameters
----------

and

Returns
-------

I missed the discussion in the thread. is this a regular pandas data type? Is there any reason to not use the dtype str?

(the comments below do not seem to help me too much, sorry)

This is an available dtype in pandas to mention that a column is a categorical column.

Is there any reason to not use the dtype str?

Actually I don't see why categories should be string :). Right now, the liac-arff does not allow otherwise, we could actually think about converting silently the categories to string if a column is of category dtype.

Cool, thanks for the clarification :)

why not use dtype=bool?

because we are using the pd.api.types.infer_dtype function which return string.

This seems to imply that internally we store everything as arff. Wouldn't it be much cooler to store everything internally as pandas, that will make it much easier to do operations on the datasets. (And of course convert before uploading dataset / performing a run). However maybe that should be a next PR.

I would say that this is another PR.

I would imagine that it would require quite some changes but if the consensus is to use pandas then why not. Then, the data should be only tabular data (which is probably the main use case in openml)

Sparse data might be an issue, right?

Sparse arrays do not have values attribute so we should be fine.

If you think about SparseDataFrame, I would say that we should not offer support. They are going to be deprecated in favor of sparse dtype.

Is the liac-arff actually usable with sparse data?

Yes, it can handle scipy's COO data structure.

So this will not be an issue. @amueller could you elaborate on your thoughts.

There's no assert for this, right?

line 797, we checked that the attributes contain 'g' which is not present in the original data. I would consider that as an assert. Do you want to test something else?

Do you mean this?

        self.assertEqual(
            _get_online_dataset_arff(upload_did),
            dataset._dataset,
            "Uploaded ARFF does not match original one"
        )

Either I'm missing something, or there is no strict check that 'g' is in the attributes of the downloaded file? Line 797 only checks that the download is equal to the upload, right?

OK I see we can add a strict check then.

Actually we could support SparseDataFrame for now. Calling to_coo instead of values should be enough then.