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

Feature: make the analysis agnostic to the indexing of the time series #4

Open
adrifcid opened this issue Jun 2, 2023 · 2 comments

Comments

@adrifcid
Copy link

adrifcid commented Jun 2, 2023

Right now, input time series have to be Pandas Series objects indexed by days: it would be nice to have the option of passing objects indexed by other time units, and even unindexed np.ndarray's, as input.

AlFontal added a commit that referenced this issue Jun 7, 2023
Just testing in python 3.10 by now
@AlFontal
Copy link
Owner

AlFontal commented Jun 7, 2023

Hi @adrifcid,

Indeed, the way the SDCAnalysis class is defined right now, only a pd.Series indexed by a datetime is able to be instantiated. The main function that actually computes sdc, which is compute_sdc is already agnostic, but the class and its instances make this assumption.

So far, I have given a hacky solution which basically forces numpy arrays or lists into pd.Series and gives them a datetime index starting at '2000-01-01'. Works but is incredibly hacky. Check the feat-test/abstract branch where I have started to implement some of these changes. It's essential to have good tests to enable us to both realize these obvious problems and also to make sure further development doesn't make us regress.

Check the tests/test_sdcpy.py file on that branch to see some of the trivial tests I added. They need for sure to be expanded, but at least it's a start.

I'd be more than happy to review and merge a PR if you feel like doing it :)

AlFontal added a commit that referenced this issue Jun 8, 2023
Just testing in python 3.10 by now
AlFontal added a commit that referenced this issue Jun 8, 2023
Just testing in python 3.10 by now
@AlFontal
Copy link
Owner

AlFontal commented Jun 8, 2023

Hi @adrifcid,

You will see that I have addressed this in a very hacky way at this point. I have implemented some changes in the repository that I had been wanting to do for a very long time.

You should see how the new release (v0.3.0) that is now available in Pypi doesn't throw an error anymore for lists or numpy arrays. It will, however, still crash for datetime indexed series with frequencies other than daily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants