-
Notifications
You must be signed in to change notification settings - Fork 733
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
initial commit for aka.ms/econml doc migration #640
Conversation
83c8d66
to
7055354
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Maybe only I would try to place the three figures in the main page of our econml project webpage, on flexbiilit ..., in the intro page of the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Added one minor comment about image hosting.
doc/spec/overview.rst
Outdated
<div class="ms-grid " style = "text-align: left; box-sizing: border-box; display: block; margin-left: auto; margin-right: auto; max-width: 1600px; position: relative; padding-left: 0; padding-right: 0; width: 100%;"> | ||
<div class="ms-row" style = "text-align: left; box-sizing: border-box; -webkit-box-align: stretch; align-items: stretch; display: flex; flex-wrap: wrap; margin-left: 3px; margin-right: 3px;"> | ||
<div class="m-col-8-24 x-hidden-focus" style = "text-align: left; box-sizing: border-box; float: left; margin: 0; padding-left: 1vw; padding-right: 1vw; position: relative; width: 33.33333%;"> | ||
<p style="text-align:center;"><img loading="lazy" class="size-full wp-image-656358 aligncenter x-hidden-focus" src="https://www.microsoft.com/en-us/research/uploads/prod/2020/05/imgFlexible.png" alt="Flexible icon" width="92" height="92"></p><p style="text-align: center"><b>Flexible</b></p><p class="x-hidden-focus">Allows for flexible model forms that do not impose strong assumptions, including models of heterogenous responses to treatment.</p><p> </p></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to copy the images locally rather than accessing them from www.microsoft.com.
@@ -10,7 +10,6 @@ __pycache__/ | |||
*.log | |||
*.out | |||
*.synctex.gz | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is fine.
If you need to make any further changes to this PR, you might consider also changing azure-pipelines.yml to exempt .gitignore changes so that you don't have to run the full set of checks, since all of your other changes are in the doc subdirectory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sphinx-doc/sphinx#10705
Because of issues with sphinx 5.1.0
@@ -71,7 +71,7 @@ jobs: | |||
- script: 'pip install git+https://github.com/slundberg/shap.git@d1d2700acc0259f211934373826d5ff71ad514de' | |||
displayName: 'Install specific version of shap' | |||
|
|||
- script: 'pip install sphinx sphinx_rtd_theme' | |||
- script: 'pip install sphinx!=5.1.0 sphinx_rtd_theme' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^
Currently there is some useful documentation on the Microsoft EconML project page but it can be hard to find. Migrating the information over to the package docs should increase visibility.