[ENH] Allow object dtype in series

[yarnabrina]

Reference: #5867 (comment)

[yarnabrina]

@fkiraly I see a new failure already, but I don't know what part of my change is affecting this. Can you guide?

Also, I have a question. What is the difference between mtype and scitype?

[fkiraly]

scitype = abstract data type, e.g., time series, or collection of time series
mtype = full python specification

[fkiraly]

The failures are due to a secondary problem, namely that objects of mtype nested_univ now also pass the check for pd.DataFrame, in addition to the nested_univ check.

That means the two mtypes are no longer distinguishable by checks, which leads to further trouble as the data checking layer can no longer decide what mtype it shoud assume

To fix this, we need to ensure tha nested_univ does not pass the pd.DataFrame check.

An idea would be to check that, in object typed columns, the first row entry is not a pd.Series.

[fkiraly]

should I try to do that?

[yarnabrina]

I'm unlikely to any time this week, so please do if you have time. I'll attempt over weekend otherwise.

…

On Mon, Feb 5, 2024, 21:36 Franz Király ***@***.***> wrote: should I try to do that? — Reply to this email directly, view it on GitHub <#5886 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJMCQBD5YMLVE4G26S2D4RDYSD7RVAVCNFSM6AAAAABCYAU4BCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRXGMZTQNRTGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[fkiraly]

@yarnabrina, your remote is rejecting push. You need to check the box "allow maintainers to make changes".

To fix, add following lines starting line 66 of _check._series:

    # check to delineate from nested_univ mtype (Panel)
    # pd.DataFrame mtype allows object dtype,
    # but if we allow object dytpe with pd.Series entries,
    # the mtype becomes ambiguous, i.e., non-delineable from nested_univ
    if isinstance(obj.iloc[0, 0], (pd.Series, pd.DataFrame)):
        msg = f"{var_name} cannot contain nested pd.Series or pd.DataFrame"
        return ret(False, msg, None, return_metadata)

[yarnabrina]

You need to check the box "allow maintainers to make changes".

Done, can you please check now?

[fkiraly]

ok, works

[yarnabrina]

@fkiraly I'll try to get back to this PR on the weekend. Can you give some suggestions about the new errors? I definitely did not expect IndexError to come after allowing a new dtype.

[fkiraly]

Can you give some suggestions about the new errors? I definitely did not expect IndexError to come after allowing a new dtype.

tried to fix - failure is incomplete edge case treatment of empty data frames (zero rows or cols)

[yarnabrina]

@fkiraly making it as ready, all tests pass after that additional check you added.

[fkiraly]

So, works now and I'd be happy with it

The change I made was the additional check in now lines 66-72 of _series._check, which allows the system to delineate nested_univ from pd.DataFrame mtype, at least in all test cases. Empty pd.DataFrame count as the latter.

[fkiraly]

actually, I noticed multiple things that imo we need to address before this:

• Panel and Hierarchical types still forbid object
• we are not systematically testing this input type - from now, users will not receive clear warning.
• possibly, many estimators will break if given object dtype-d data - we need to think about what to do. Options: some kind of coercion; having estimator tags which tell the user which dtypes are supported, and an informative message is raised based on that.
  • none seems fully satisfactory, as this is very pandas specific.

[yarnabrina]

Panel and Hierarchical types still forbid object

I actually skipped this one intentionally. I thought it may be better to allow separately for different types. Unless you think they are interlinked and can cause issues, I really have very little, if any, familiarity with how scitype/mtype/etc. works in sktime?

many estimators will break if given object dtype-d data

This one is sort of a dilemma - currently a lot can use object type and we don't let them use it, while allowing may break some which do support. I don't know how to choose. Also, there's a "category" type which is being more and more popular. pandas has recently planning to use even a dedicated "string" type, I think I saw in their latest release notes.

we are not systematically testing this input type - from now, users will not receive clear warning.

I didn't understnad what you meant by this one. What is happening now, and what will change to confuse users?

[fkiraly]

Unless you think they are interlinked and can cause issues

Yes, they are linked through broadcasting.

One potential failure case that is not covered due to lack of examples for the contract expansion is as follows:

suppose we have a transformer that internally is implemented only for Panel (e.g., for efficiency reasons).
Upon receiving a Series, it will be upcast to Panel, and suddenly the object no longer satisfies the contract.

Similarly, there is now an inconsistency the other way round - even if an estimator would now accept an individual series, it would no longer accept it if it is found with others of similar type in a collection of series.

PS: I think for Panel, the trick with the iloc is not needed, as the delineation happens via the MultiIndex already.

[fkiraly]

This one is sort of a dilemma - currently a lot can use object type and we don't let them use it, while allowing may break some which do support. I don't know how to choose. Also, there's a "category" type which is being more and more popular

I do think ew should support it, but "properly".
Here's what I suggest:

• extend the types for Panel and Hierarchical as well
• in scenarios_forecasting and scenarios_transformers add coverage for this
  • if not too much breaks, hooray - fix the few bugs etc
  • if a lot breaks, we need to consider how we make the contract tight. A way would be introducing tags for the supported types.

[fkiraly]

we are not systematically testing this input type - from now, users will not receive clear warning.

I didn't understnad what you meant by this one. What is happening now, and what will change to confuse users?

Consider the situation of an estimator that does not support the type genuinely, i.e., would break internally of pd.DataFrame with object dtype is passed.

Suppose a user passes a pd.DataFrame with object dtype to that estimator. Currently, on main, this will fail with an informative error message, namely "object dtype is not supported" or similar. This is because the check catches it, and also returns an informative error message together with the check.

However, if we widen the contract without testing and adding catches where they fail, then in this estimator something random will break, and the error will be uninformative, e.g., a ValueError or type error or similar. This is of course not good.

From a higher poin of view, by widening the contract, we may now be creating uncovered bugs. So either we have to move the contract boundary "further in" by one of the measures proposed above (e.g., tag and catch), or we have to ensure all estimators satisfy the new contract. In any case, it has to be tested.

[fkiraly]

well, that's strange. One of the notebooks fails, while the test suite is fine.

We need to identify what object is being passed to the checkers, and why it is ambiguous. Perhaps it has mixed columns?

[fkiraly]

oh wait, the problem is actually Panel. I am reverting the change I made to the Series checks.

We just need to add the same trick in pd-multiindex checker.

For Hierarchical, this will not be needed.

[fkiraly]

to avoid conflicts: do you want ot add the mini-check in Panel, or should I?

[yarnabrina]

Please do. I've very little experience and confidence in this datatypes modules.

I just faced weird set of conflicts. Fixed locally, so let me make one last attempt.

[yarnabrina]

I did a rebase from main and force push, so I think you will get conflicts. So, if possible can you delete your local branch and pull this?

Also, I do not know where are the other changes you said are necessary to do, so can you please apply that? I'll not modify this branch anymore to avoid future conflicts, deleting from my local.

[yarnabrina]

I just copy pasted your comment and tried to edit two terms, but I'm certain this is wrong. So, please review and change.

[fkiraly]

that should work though, let's see what the tests say