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

convert tsfeatures lib to nbdev framework #39

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

Conversation

jope35
Copy link

@jope35 jope35 commented Jan 4, 2024

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 👍

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

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)

tsfeatures/__init__.py Show resolved Hide resolved
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
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.
@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.

Copy link
Author

jope35 commented Feb 4, 2024

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

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
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
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
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

#!/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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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

@@ -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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants