-
Notifications
You must be signed in to change notification settings - Fork 2
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
docs: add examples of replacing providers #101
Conversation
docs/user-guide/index.md
Outdated
@@ -8,4 +8,5 @@ maxdepth: 2 | |||
getting-started | |||
parameter-tables | |||
generic-providers | |||
replacing-providers |
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.
I suggest we add this in Recipes
, instead of User Guide
"id": "86de99b0-3170-45d6-84eb-adbd622af936", | ||
"metadata": {}, | ||
"source": [ | ||
"# Replacing providers\n", |
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 is not exactly what the issue was about. I think insert
and __setitem__
can be found in the API docs. The issue was about documenting how to continue from intermediate results. Someone who wants to do that might not look for "replacing providers" (if they would, then they would already know how to do that). I think we should make this clear in the title, and maybe stick to the first example on this page (linking the insert
can be useful).
If you want to keep the "replacing provider" example, I'd suggest to make this into another recipe?
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.
Sounds good!
docs/recipes/recipes.ipynb
Outdated
"id": "60f9b2a8-0557-43da-a8d2-41df2794bd49", | ||
"metadata": {}, | ||
"source": [ | ||
"## Continue from intermediate results\n", |
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.
Hmm, maybe we should split this page into individual notebooks?
docs/recipes/recipes.ipynb
Outdated
"It is a common need to be able to continue the pipeline from some intermediate result computed earlier.\n", | ||
"\n" | ||
] | ||
}, | ||
{ | ||
"cell_type": "markdown", | ||
"id": "aa1df625-9f6a-44d3-9169-1c79ff8e647f", | ||
"metadata": { | ||
"jp-MarkdownHeadingCollapsed": true | ||
}, | ||
"source": [ | ||
"### Setup\n", |
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.
Before the full and working example, should we give a TL;DR?
data = pipeline.compute(CleanData)
pipeline[CleanData] = data
result = pipeline.compute(Result)
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.
Yeah that makes sense
docs/recipes/recipes.ipynb
Outdated
"But if the cleaned data has already been produced it is unnecessary to \"re-clean\" it, in that case we can proceed directly from the clean data to the compute sum step.\n", | ||
"To do this we replace the `CleanData` provider with the data that was loaded and cleaned:" |
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 is a bit confusing, because the computation of CleanData
comes after the above line, where you say we replace the provider. A novice user might thing the call to compute
is part of the replacement. How about something like:
Given a pipeline, we may want to compute an intermediate result for inspection:
data = pipeline.compute(CleanData)
If later on we wish to compute a result further down the pipeline (derived from CleanData
), this would cause potentially very costly re-computation of CleanData
, since Sciline does not perform any caching:
result = pipeline.compute(Result) # re-computes CleanData
To avoid this, we can use Pipeline.__setitem__
to replace the provider of CleanData
by the previously computed data:
pipeline[CleanData] = data
result = pipeline.compute(Result) # uses data as CleanData
docs/recipes/recipes.ipynb
Outdated
"metadata": {}, | ||
"source": [ | ||
"### Setup\n", | ||
"Same setup as in [Continue from intermediate results](#continue-from-intermediate-results)." |
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.
Let us avoid this, recipes should be stand-alone.
"id": "d7608bb4-d7ce-4a36-8021-f1a52ad27ec5", | ||
"metadata": {}, | ||
"source": [ | ||
"# Recipes" |
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.
Need to change section heading levels below, and also all the other files.
Co-authored-by: Simon Heybrock <12912489+SimonHeybrock@users.noreply.github.com>
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.
You need to fix docs/index.md
, Recipes does not show up any more.
Fixes #97
replacing-providers.pdf