fix: simplify public-facing types by inlining type-aliases

Aarhus-Psychiatry-Research / timeseriesflattener

Converting irregularly spaced time series, such as eletronic health records, into dataframes for tabular classification.

https://Aarhus-Psychiatry-Research.github.io/timeseriesflattener

MIT License

19 stars 2 forks source link

fix: simplify public-facing types by inlining type-aliases #514

Closed HLasse closed 5 months ago

HLasse commented 8 months ago

E.g. init_df of ValueFrame|TimestampValueFrame takes an InitDF_T which is hard to parse without a doc string specifying it or by changing the type to be human readable.

Others:

[x] look[behind|ahead] distance: LookDistances (sequence of dt.timedelta)
[x] fallback: ValueType
[x] entity_id_col_name: default_entity_id_col_name (instead of the string repr)
[x] default_pred_time_col_name
[x] default_prediction_time_uuid_col_name
[x] default_timestamp_col_name

github-actions[bot] commented 7 months ago

This issue is stale because it has been open for 14 days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

github-actions[bot] commented 7 months ago

This issue is stale because it has been open for 14 days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

github-actions[bot] commented 7 months ago

This issue was closed automatically. Feel free to re-open it if it's important.

github-actions[bot] commented 6 months ago

This issue is stale because it has been open for 14 days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.

HLasse commented 6 months ago

@MartinBernstorff are you on board with inlining the type aliases? As we discussed, it makes it easier for the user, but a bit less nice for developers. I think it's a worthy trade-off in this case, though

MartinBernstorff commented 6 months ago

I'm uncertain how many type-aliases will be inlined.

It does make me a little uncomfortable that types which are supposed to be the same can drift from one another.

MartinBernstorff commented 6 months ago

If you have a strong opinion, I'm OK with that 👍

HLasse commented 6 months ago

I've added a quick PR (#564) for the types outlined above to get a feel for how it would look. Some of the types were only used in one place, but I completely get the worry that more widely used types can potentially drift. It does it make it substantially easier for users, though. Have a look and let's talk about it at some point

github-actions[bot] commented 5 months ago

This issue is stale because it has been open for 14 days with no activity. Feel free to either 1) remove the stale label or 2) comment. If nothing happens, this will be closed in 7 days.