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.

Sign up for GitHub

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

Jump to bottom

convert tsfeatures lib to nbdev framework #39

Open
wants to merge 53 commits into
base: main
Choose a base branch
from
Open

convert tsfeatures lib to nbdev framework #39

wants to merge 53 commits into from

Conversation

jope35
Copy link

@jope35 jope35 commented Jan 4, 2024 •
edited
Loading

This pr uses the nbdev framework to build the tsfeatures lib.
All old files are copied 1-on-1, eager to get some feedback on this

@jope35 jope35 changed the title convert lib to nbdev convert tsfeatures lib to nbdev framework Jan 4, 2024
@jmoralez
Copy link
Member

jmoralez commented Jan 9, 2024

Hey @jope35, thanks for the initiative, we support this. Just some requests before we review this more thoroughly:

  • Please restore the files under the .github folder.
  • Please restore the .gitignore, LICENSE and setup.py files.
  • Please remove the pre-commit-config.yaml and .ruff.toml. These can be added in a later PR so that we can focus on those changes specifically.
  • Please restore and move the compare_with_r.py file to a scripts folder or similar.

@jope35
Copy link
Author

jope35 commented Jan 10, 2024

@jmoralez thanks for the quick scan. will incorporate the changes 👍

@review-notebook-app Review Notebook App
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

@jmoralez
Copy link
Member

jmoralez commented Jan 16, 2024 •
edited
Loading

Some more requests:

  • Please update the settings.ini file with the current settings in setup.py, e.g. version should be 0.4.5. For the author and other stuff you can refer to this file.
  • Please update the setup.py file. I believe you can copy this one
  • Please remove the ordering from the notebooks (the number prefixes)

jmoralez
jmoralez requested changes Jan 16, 2024
View reviewed changes
tsfeatures/__init__.py Show resolved Hide resolved
jmoralez
jmoralez reviewed Jan 16, 2024
View reviewed changes
nbs/00_utils.ipynb Outdated Show resolved Hide resolved
nbs/00_utils.ipynb Outdated Show resolved Hide resolved
nbs/00_utils.ipynb Outdated Show resolved Hide resolved
nbs/01_features.ipynb Outdated Show resolved Hide resolved
@jope35
remove *.ipynb from gitignore
846508e
@jope35
align settings and setup
bf80383
@jope35
refactor: Restructure notebooks and utils
ed0266f 
Renamed and reorganized notebooks for clarity. Moved some utils to hide cells. Updated version and doc links. Added script to compare features with R. Made other minor code tweaks.
@jope35
update settings with nixtla user
ddabac9
@jmoralez
Copy link
Member

Thanks for the updates. Can you please merge tsfeatures_core.ipynb with features.ipynb? They both should be the same notebook that exports the tsfeatures module.

@jope35 Review Notebook App
Copy link
Author

jope35 commented Feb 4, 2024

Merge tsfeatures_core.ipynb with features.ipynb, the result is exported to tsfeatures.

jmoralez
jmoralez requested changes Feb 6, 2024
View reviewed changes
Copy link
Member

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add a workflow that runs nbdev_test. You can refer to this

setup.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
tsfeatures/tsfeatures.py Outdated Show resolved Hide resolved
jmoralez
jmoralez requested changes Feb 15, 2024
View reviewed changes
Copy link
Member

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also move the test from the test_features notebook to the tsfeatures one

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

To replicate this results use:
[![Build](https://github.com/FedericoGarza/tsfeatures/workflows/Python%20package/badge.svg)](https://github.com/FedericoGarza/tsfeatures/tree/master)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like from this point onwards this is duplicated

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

de-duplicated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still duplicated

requirements.txt Outdated Show resolved Hide resolved
settings.ini Outdated Show resolved Hide resolved
jmoralez
jmoralez requested changes Feb 27, 2024
View reviewed changes
Copy link
Member

@jmoralez jmoralez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes! I think we're almost there

tsfeatures/tsfeatures.py Show resolved Hide resolved
jmoralez
jmoralez requested changes Mar 20, 2024
View reviewed changes
README.md Outdated

To replicate this results use:
[![Build](https://github.com/FedericoGarza/tsfeatures/workflows/Python%20package/badge.svg)](https://github.com/FedericoGarza/tsfeatures/tree/master)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still duplicated

tsfeatures/tsfeatures_r.py
#!/usr/bin/env python
# coding: utf-8

from typing import List
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add this to a new notebook nbs/tsfeatures_r.ipynb?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added the notebook and extended the ci run to include R also added rpy2 in the dependencies list, @jmoralez maybe move it to the dev dependencies as it gives some troubles installing on windows

tsfeatures/tests/test_holt_parameters.py
@@ -1,18 +0,0 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

tsfeatures/tests/test_mutability.py
@@ -1,31 +0,0 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

tsfeatures/tests/test_pacf_features.py
@@ -1,10 +0,0 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

tsfeatures/tests/test_small_ts.py
@@ -1,36 +0,0 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

tsfeatures/tests/test_sparsity.py
@@ -1,25 +0,0 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

tsfeatures/tests/test_statistics.py
@@ -1,25 +0,0 @@
#!/usr/bin/env python
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add these tests to nbs/tsfeatures.ipynb

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the tests to the tsfeatures.ipynb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmoralez jmoralez jmoralez requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jope35 @jmoralez