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

Further doc updates #54

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Further doc updates #54

wants to merge 21 commits into from

Conversation

jack-fs
Copy link
Contributor

@jack-fs jack-fs commented May 9, 2023

This PR will contain additional documentation updates and address comments from #53

@jack-fs
Copy link
Contributor Author

jack-fs commented May 12, 2023

CI tests are failing due to unrelated issues as documented in #55. Tests and linting pass on my machine.

This PR now involves:

  • substantial updates to the documentation of
    • dependence.py
    • evaluators.py
    • model_evaluation.py
  • proof-reading and editing of many other sections of documentation;
  • new Makefile docs target for html docs, for convenience;
  • no substantial code changes; and
  • new sphinx documentation for the simulations module.

Most comments in #53 would involve substantial actual code changes, so should probably be addressed separately

@jack-fs jack-fs marked this pull request as ready for review May 12, 2023 05:04
Copy link
Contributor

@dsteinberg dsteinberg left a comment

Choose a reason for hiding this comment

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

This is great Jack! You've put in a lot of great work!

I only have one comment in this review about cv. Otherwise some things to consider:

  • Include the README in the docs front page (as we did in aboleth) so users are given a nicer front page other than just the API reference
  • host the docs on readthedocs, and make a hook in the CI that rebuilds these docs and uploads it to readthedocs

Otherwise I'll leave it to Simon and Al :-)

README.md Outdated
@@ -103,7 +103,8 @@ pieval = PermutationImportanceEvaluator(n_repeats=5)
# Bootstrap sample the data, re-fitting and re-evaluating the model each time.
# This will run the GridSearchCV estimator, so thereby performing model
# selection within each bootstrap sample.
bootstrap_model(best_model, X, Y, [pdeval, pieval], replications=30)
# n_jobs=-1 parallelises the bootstrapping to use all cores.
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

evaluators : Sequence[Evaluator]
A list of evaluators.
cv : Union[int, BaseCrossValidator], optional
The cross validation strategy, by default KFold(n_splits=5)
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be worth mentioning that if this is of type int, it results in KFold(n_splits=cv) as per scitkit-learn's interface

@jack-fs
Copy link
Contributor Author

jack-fs commented May 17, 2023

  • Update model_evaluation.crossval_model docs
  • Add README to sphinx docs
  • Publish on readthedocs: https://causal-inspection.readthedocs.io
    • Automatic doc builds on pull requests can be configured as here. This requires some degree of permissions. It seems possible to set up CI manually using GitHub actions, but we would need to be able to store a webhook token as a secret in the repo, which requires admin access

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.

None yet

2 participants