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

Time-series Forecast ZenFile #35

Merged
merged 52 commits into from Jun 21, 2022
Merged

Time-series Forecast ZenFile #35

merged 52 commits into from Jun 21, 2022

Conversation

lukyrasocha
Copy link
Contributor

Predict electricity power generation from wind forecast using ZenML:

  • Load data directly from Google Cloud BigQuery
  • Train a model remotely in Vertex AI

Also contains a thorough description on how to set up and configure a google cloud project together with a zenml stack.

@lukyrasocha lukyrasocha changed the title zenfiles/time-series-forecast Time-series-forecast Zenfile Jun 2, 2022
@strickvl strickvl added the enhancement New feature or request label Jun 2, 2022
@lukyrasocha
Copy link
Contributor Author

The spell check is failing on 'iam', but it is an abbreviation of Identity and Access Management.

@htahir1 htahir1 requested a review from ayush714 June 2, 2022 14:40
@htahir1
Copy link
Contributor

htahir1 commented Jun 2, 2022

@strickvl any possibility to add iam to the spell check ignore?

@strickvl
Copy link
Contributor

strickvl commented Jun 2, 2022

@strickvl any possibility to add iam to the spell check ignore?

I would probably just ignore it for now. We're going to replace codespell spellchecks with pyspelling in the ZenFiles pretty soon.

Copy link
Contributor

@ayush714 ayush714 left a comment

Choose a reason for hiding this comment

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

Hi,

First of all, It's an amazing ZenFile which showcases the power of ZenML, I do have some changes requested, but in overall I think:-

  • It would be good if you write good explanation of every step which you write in README.md.
  • I think you haven't added doc strings in your code, so add comments in every step and pipelines.
  • I think it would be nice if you can be standardised a bit with other ZenFiles. Like the structure of the project, I think you can organise this project in better way.
  • I also feel README.md is bit empty, so useful explanations would be nice.
  • I think for setting up this project, I need to have poetry, but I think you should give a traditional option as well for setting up this project just like we did in other ZenFiles because that is bit common. I also think you can reference a link which says how one can install poetry and use poetry for installation.

If I think there can be any other changes, I will add other reviews for sure. If you are complete with current feedbacks, you can re request me for reviewing it again.

Amazing work :)

time-series-forecast/README.md Outdated Show resolved Hide resolved
time-series-forecast/README.md Outdated Show resolved Hide resolved
time-series-forecast/README.md Outdated Show resolved Hide resolved
time-series-forecast/src/main.py Outdated Show resolved Hide resolved
time-series-forecast/src/main.py Outdated Show resolved Hide resolved
time-series-forecast/README.md Outdated Show resolved Hide resolved
time-series-forecast/README.md Show resolved Hide resolved
time-series-forecast/src/steps/transformer.py Show resolved Hide resolved
X_train=np.ndarray, X_test=np.ndarray, y_train=np.ndarray, y_test=np.ndarray
):
df = data.copy()
cardinal_directions = {'N': 0.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks a bit long, what about adding it in config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code a bit so it looks better and added a description of how the feature engineering works. I find it more clear if the code stays together instead of using a config file in this case

time-series-forecast/README.md Outdated Show resolved Hide resolved
Co-authored-by: Ayush Singh <81796368+ayush714@users.noreply.github.com>
@lukyrasocha
Copy link
Contributor Author

Hey @ayush714, thank you so much for the feedback. I tried to change and adjust all the things you mentioned, but please let me know any additional feedback that you might have. Thanks!

@lukyrasocha lukyrasocha requested a review from ayush714 June 8, 2022 12:25
Copy link
Contributor

@ayush714 ayush714 left a comment

Choose a reason for hiding this comment

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

It looks good from my side, I will wait for other peoples to approve it maybe @AlexejPenner and @strickvl.

@strickvl strickvl changed the title Time-series-forecast Zenfile Time-series Forecast ZenFile Jun 9, 2022
@AdamVPro AdamVPro requested a review from ayush714 June 10, 2022 09:13
@ayush714
Copy link
Contributor

Hi, I will run your whole code on my system and see everything work as per your readme instructions. I will be done by 14th June ( most probably ).

Copy link
Contributor

@ayush714 ayush714 left a comment

Choose a reason for hiding this comment

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

I again did a small review, I have a comment in README, I see that you are starting from very scratch which is good, what if I already have those things set up? I just need to create my stack, so in that case, it might get confused for other peoples who have things setup, so it would be nice to have a general note on the steps which starts from scratch.

I will be running your code once and will give more feedback.

time-series-forecast/requirements.txt Outdated Show resolved Hide resolved
time-series-forecast/requirements.txt Outdated Show resolved Hide resolved
lukyrasocha and others added 3 commits June 16, 2022 12:02
Co-authored-by: Ayush Singh <81796368+ayush714@users.noreply.github.com>
Co-authored-by: Ayush Singh <81796368+ayush714@users.noreply.github.com>
Add a description of how to upload the data set to bigquery from CLI and also separated GCP steps and Zenml steps into two separate sections based on the latest comments
@lukyrasocha
Copy link
Contributor Author

In the readme I now separated the steps into two sections - Steps on how to set up a GCP project and Steps on how to set up components for zenml stack. That should clear things up. I also added a description on how to upload the data set into BigQuery from CLI

@ayush714 ayush714 self-requested a review June 20, 2022 10:55
Copy link
Contributor

@ayush714 ayush714 left a comment

Choose a reason for hiding this comment

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

I ran it on my own system and it seems to be working, I didn't found any such issues, It will be helpful if you can add a diagram/pipeline diagram in high level so it will be easier for peoples to understand what's going on!?

However I don't feel a strong requirement for that, if u can it's good otherwise I am approving it.

Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

I love this! Very well explained and a valuable showcase. Thank you so much for the amazing contribution @lukyrasocha !

@htahir1 htahir1 merged commit 7dc7680 into zenml-io:main Jun 21, 2022
@htahir1
Copy link
Contributor

htahir1 commented Jun 21, 2022

Ok im merging it now. Amazing contribution and we'll be happy to release this tomorrow to the world!

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

Successfully merging this pull request may close these issues.

None yet

4 participants