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

support to_data\from_data for DictInterval #51

Closed
wants to merge 3 commits into from

Conversation

Zagrebelin
Copy link

I suppose to "serialize" DictIntevals to the list of 2-tuples ( (left, lower, upper,right), item ).

I suppose to "serialize" DictIntevals to the list of 2-tuples  `( (left, lower, upper,right), item )`.
@Zagrebelin
Copy link
Author

I think I have to add some tests here.

@AlexandreDecan
Copy link
Owner

Thanks for your PR! I will have a look asap and provide some feedback :)

@AlexandreDecan AlexandreDecan added the enhancement New feature or request label Dec 9, 2020
@AlexandreDecan
Copy link
Owner

First of all, thank you for your contribution. I see that the purpose of this PR is to provide an "easy way" to import and export IntervalDict instances to Python's built-in datatype.

However, I don't really understand why this would be part of the library given that one can, quite easily, re-use the fundamentals provided by the library to do so (for instance, I would expect an IntervalDict to be exported using {P.to_data(k, ...): v for k, v in intervaldict.items()} and to be imported using a similar approach, e.g. IntervalDict._from_items({P.from_data(k, ...): v for k,v in d.items()})). Could you elaborate a little bit on the need to include these routines into the library?

Thank you! ;-)

@Zagrebelin
Copy link
Author

I using your library in my current project to handle time and datetime intervals of some events in Django application.
It was very easy with to_data\from_data and jsonfield.
Today i received a new task to store some extra information within intervals, and a IntervalDicts with time intervals as keys and dict as values are works very well for me. There are a routines for store Intervals to plain object, but there is nothing to store IntervalDict so i suggest this PR.

Yes, you are right. I submit a stupid non working code, it was my fault.

@AlexandreDecan
Copy link
Owner

It's not stupid at all ;-)

I was mainly wondering whether this should be part of the library or not.

@Zagrebelin
Copy link
Author

As far we have a to_data/from_data for simple Intervals I think there should be dict_to_data. Just for unification.

But this is your library and your decision.

@AlexandreDecan
Copy link
Owner

Indeed, but the reason why we have from_data/to_data is simply because there is no easy alternative to offer a similar service than having these functions. What I mean is that an IntervalDict can be exported and imported the same way than any other dict in Python. The only "issue" would be to convert the keys (that are Interval objects) to something serializable, and that can be easily done with from_data/to_data.

The changes you made for 1674b17 show that it is easy to write such import/export without having to rely on some internals, that's why I'm hesitating to merge this PR.

@AlexandreDecan
Copy link
Owner

We have discussed a lot on whether we should bring this functionality to portion or not, and have concluded that we shouldn't. The IntervalDict class is mostly a convenient class to map data to intervals and, as such, is kind of a third-party feature of portion. If at some point, we decide to move IntervalDict to the main set of features, we'll reconsider our decision.

Thanks again for this PR, I'm sorry for its fate.

@AlexandreDecan AlexandreDecan added the wontfix This will not be worked on label May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants