Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Various documentation fix#477

Merged
xiaoyongzhu merged 29 commits into
mainfeathr-ai/feathr:mainfrom
xiaoyzhu/doc_fix2feathr-ai/feathr:xiaoyzhu/doc_fix2Copy head branch name to clipboard
Aug 2, 2022
Merged

Various documentation fix#477
xiaoyongzhu merged 29 commits into
mainfeathr-ai/feathr:mainfrom
xiaoyzhu/doc_fix2feathr-ai/feathr:xiaoyzhu/doc_fix2Copy head branch name to clipboard

Conversation

@xiaoyongzhu
Copy link
Copy Markdown
Member

@xiaoyongzhu xiaoyongzhu commented Jul 15, 2022

Various documentation fix per feedback from end users, covering those issues:
#437
#464
#461
#473 (part of it)

This PR also rewrites the feature join doc, fixing various typos, and add csv support for get_result_df helper function per one of user's feedback.

Comment thread docs/concepts/feathr-concepts-for-beginners.md Outdated
Comment thread docs/concepts/materializing-features.md Outdated
Comment thread feathr_project/feathr/definition/transformation.py Outdated
Comment thread docs/concepts/get-offline-features.md
Comment thread docs/concepts/materializing-features.md
@blrchen
Copy link
Copy Markdown
Collaborator

blrchen commented Jul 20, 2022

                               feature_names=["f_location_avg_fare", "f_location_max_fare"],

The doc for get_offline_features uses product recommendation data but the doc for materilizate_features uses NYC taxi data. This might confuse readers.

I know consolidate this mgiht not be intention of this PR am okay this be addressed in different PR.


Refers to: docs/concepts/materializing-features.md:44 in 85a1c44. [](commit_id = 85a1c44, deletion_comment = False)

Comment thread docs/concepts/get-offline-features.md
@xiaoyongzhu
Copy link
Copy Markdown
Member Author

                               feature_names=["f_location_avg_fare", "f_location_max_fare"],

The doc for get_offline_features uses product recommendation data but the doc for materilizate_features uses NYC taxi data. This might confuse readers.

I know consolidate this mgiht not be intention of this PR am okay this be addressed in different PR.

Refers to: docs/concepts/materializing-features.md:44 in 85a1c44. [](commit_id = 85a1c44, deletion_comment = False)

Yeah we need to update all the samples quite a bit so let's do it in a separate PR.

Comment thread docs/concepts/feathr-concepts-for-beginners.md Outdated
Comment thread feathr_project/feathr/utils/job_utils.py Outdated
Comment thread feathr_project/feathr/utils/job_utils.py Outdated
Comment thread docs/quickstart_databricks.md Outdated
Comment thread docs/quickstart_databricks.md Outdated
Comment thread feathr_project/feathr/utils/job_utils.py Outdated
Yuqing-cat
Yuqing-cat previously approved these changes Aug 1, 2022
Comment thread feathr_project/feathr/utils/job_utils.py Outdated
Comment thread feathr_project/test/test_azure_spark_e2e.py
@xiaoyongzhu xiaoyongzhu merged commit 2a62a04 into main Aug 2, 2022
@xiaoyongzhu xiaoyongzhu deleted the xiaoyzhu/doc_fix2 branch August 2, 2022 04:34
This was referenced Aug 2, 2022
ahlag pushed a commit to ahlag/feathr that referenced this pull request Aug 26, 2022
* Update README.md

* update docs per feedback

* Update feathr-concepts-for-beginners.md

* Update feathr-concepts-for-beginners.md

* update materialization setting doc

* Update get-offline-features.md

* Update get-offline-features.md

* Update feathr-concepts-for-beginners.md

* resolve comments

* Update job_utils.py

* fix typos

* Update job_utils.py

* Update client.py

* format doc

* Address comments

* Update WriteToHDFSOutputProcessor.scala

* Update WriteToHDFSOutputProcessor.scala

* resolve comments

* Resolve comments

* fix test failures and typos

* Update job_utils.py

* fix comments and formats/typos

* fix typos and test failures

* update test names

* Update test_fixture.py

* Update test_fixture.py
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.

3 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.