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

Various documentation fix #477

Merged
merged 29 commits into from
Aug 2, 2022
Merged

Various documentation fix #477

merged 29 commits into from
Aug 2, 2022

Conversation

xiaoyongzhu
Copy link
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.

docs/concepts/feathr-concepts-for-beginners.md Outdated Show resolved Hide resolved
docs/concepts/materializing-features.md Outdated Show resolved Hide resolved
feathr_project/feathr/definition/transformation.py Outdated Show resolved Hide resolved
docs/concepts/get-offline-features.md Show resolved Hide resolved
@blrchen
Copy link
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)

@xiaoyongzhu
Copy link
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.

docs/concepts/feathr-concepts-for-beginners.md Outdated Show resolved Hide resolved
feathr_project/feathr/utils/job_utils.py Outdated Show resolved Hide resolved
feathr_project/feathr/utils/job_utils.py Outdated Show resolved Hide resolved
Yuqing-cat
Yuqing-cat previously approved these changes Aug 1, 2022
feathr_project/feathr/utils/job_utils.py Outdated Show resolved Hide resolved
@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