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

Fixed options dublicate in documentation for EmbeddingInitializerField #4022

Merged

Conversation

mhabedank
Copy link
Collaborator

The options for the EmbeddingInitializerField are displayed twice in the documentation.

This is due to the fact that both the field in the Ludwig repro and the build logic in the documentation append the options list to the description string in this particular case.

The problem has been solved by removing this logic from the field. The documentation takes over the task of appending this information.

See issue in documentation repo.

There should be a discussion about what the right path is. At the moment, different fields handle the task differently. Sometimes the documentation is supposed to assemble the string, sometimes the field does it itself.

Copy link

github-actions bot commented Aug 5, 2024

Unit Test Results

  6 files  ±0    6 suites  ±0   13m 41s ⏱️ -46s
12 tests ±0    7 ✔️  -   2    5 💤 +  2  0 ±0 
60 runs  ±0  30 ✔️  - 12  30 💤 +12  0 ±0 

Results for commit f877f75. ± Comparison against base commit 341a8bc.

This pull request skips 2 tests.
tests.regression_tests.benchmark.test_model_performance ‑ test_performance[ames_housing.gbm.yaml]
tests.regression_tests.benchmark.test_model_performance ‑ test_performance[mercedes_benz_greener.gbm.yaml]

@alexsherstinsky
Copy link
Collaborator

@mhabedank Hi! Thank you for looking into this issue. Could you please point out what the duplication issue actually is? By reading the documentation, I am seeing the embedding_initializer included in different encoder documentation pages that use it. Perhaps you could provide screenshot of the documentation and highlight the duplication you are seeing? Thank you for helping to clarify your observation.

Copy link
Collaborator

@alexsherstinsky alexsherstinsky left a comment

Choose a reason for hiding this comment

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

Requesting a clarification.

@mhabedank
Copy link
Collaborator Author

Here in category features encoding section: https://ludwig.ai/latest/configuration/features/category_features/#encoders

Bildschirmfoto 2024-08-05 um 18 26 15

@alexsherstinsky
Copy link
Collaborator

Here in category features encoding section: https://ludwig.ai/latest/configuration/features/category_features/#encoders

Bildschirmfoto 2024-08-05 um 18 26 15

@mhabedank I see -- thank you very much for highlighting. Ideally, the solution would be the one that also includes the "(default: null)" part (with everything being the same). Does your fix make it so? Thank you.

@mhabedank
Copy link
Collaborator Author

mhabedank commented Aug 6, 2024

The default part is not part of the issue. The green marked part should be gone.

That's the part generated within the dataclass field in the Ludwig repo.

@alexsherstinsky
Copy link
Collaborator

The default part is not part of the issue. The green marked part should be gone.

@mhabedank Got it. Does your fix properly delete the green part, without causing problems in other parts of the documentation? Thank you.

@mhabedank
Copy link
Collaborator Author

EmbeddingInitializerField is only used in two places within the category feature. To the best of my knowledge, I cannot see a problem.

@alexsherstinsky
Copy link
Collaborator

EmbeddingInitializerField is only used in two places within the category feature. To the best of my knowledge, I cannot see a problem.

🙇

Copy link
Collaborator

@alexsherstinsky alexsherstinsky left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you very much, @mhabedank

@alexsherstinsky alexsherstinsky merged commit d386fa5 into ludwig-ai:master Aug 6, 2024
18 checks passed
@mhabedank mhabedank deleted the fix-options-dublicate-in-docs branch August 6, 2024 15:06
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.

2 participants