Add heterogeneity indices, printing indices/legend, fix index.html.#56
Add heterogeneity indices, printing indices/legend, fix index.html.#56justushelo wants to merge 1 commit intoSimulation-Decomposition:mainfrom
Conversation
- Add heterogeneity indices function. - Add functionality for printing indices and legend. sensitivity_indices.py edited to get var_names for printing. - Added guardrails for output parameter in decomposition.py - Added tests for printing legend. - Initialize decomposition in dashboard with 0.8*sum(si). Other variables still can be chosen after this. - Updated docs to print correct second-order effects. Closes Simulation-Decomposition#46, Closes Simulation-Decomposition#47
✅ Deploy Preview for simdec-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
tupui
left a comment
tupui
left a comment
There was a problem hiding this comment.
Good start, I left some comments. We will also be missing a test for heterogeneity_indices
| def decomposition( | ||
| inputs: pd.DataFrame, | ||
| output: pd.DataFrame, | ||
| output: pd.DataFrame | np.ndarray, |
There was a problem hiding this comment.
Not a fan of adding this just on 1 parameters as we are not consistent then. Either we allow this on both or none. This is more done on the user side IMO.
What we can consider in the future is to be actually dataframe agnostic and use Narwhal. This is a great way to support multiple format at once as it supports the data API.
| print(f"{'-'*69}") | ||
| print(df_indices) | ||
| print(f"{'-'*69}") |
There was a problem hiding this comment.
Let's use one print with \n or we can even consider template strings.
| kind: Literal["histogram", "boxplot"] = "histogram", | ||
| ax=None, | ||
| print_legend: bool = False, | ||
| decomposition=None, |
| Matplotlib axis. | ||
| print_legend: Boolean, optional | ||
| Prints plot legend. | ||
| decomposition: Object, optional |
There was a problem hiding this comment.
We should specify the type, not just Object
| raise ValueError("'kind' can only be 'histogram' or 'boxplot'") | ||
|
|
||
| if print_legend: | ||
| from IPython.display import display |
There was a problem hiding this comment.
I don't think we have it declared as a dependency. This has to be added in pyproject.toml as optional and here we have to guard the input. Usually we do this at the top and use a flag to activate or not a feature.
You can also consider the new lazy import if we update the Python version.
| f"{valid}/{total_regions} regions passed all checks for '{split_name}'.\n" | ||
| f"Skipped regions:\n" | ||
| + "\n".join(f" {r!r}: n={n}, {reason}" for r, n, reason in skipped) | ||
| + "\n\nTry: (1) reducing n_subdivisions, " |
| continue | ||
|
|
||
| if skipped: | ||
| print( |
There was a problem hiding this comment.
Really not a fan of using any print. We should use the logging if we must. People using the library have no control on prints vs logging which they can properly capture and filter. That's the main reason.
| If True, displays a stacked bar chart of regional sensitivities. | ||
|
|
||
| Returns | ||
| ---------- |
| """ | ||
| Compute sensitivity-based heterogeneity across subdivisions of a variable. |
There was a problem hiding this comment.
A nits but the format is actually to always write on the first line the sentence ending with a point. If too long, we make two sentences leaving a blank line.
| @@ -0,0 +1,171 @@ | |||
| from .sensitivity_indices import sensitivity_indices | |||
There was a problem hiding this comment.
Import order matters (also sorting m before n, p) this should go at the bottom (with a blank line to separate) as this is coming from our library. Order is, std lib, 3rd party deps, dependencies from our own other packages, then the current package.
Also prefer to always use absolute import using our namespace simdec. ...
2 participants
Closes Automatic printing of indices #46, Closes Automatic legend #47