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

Cardiac tutorial #233

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

Cardiac tutorial #233

wants to merge 21 commits into from

Conversation

aranas
Copy link
Contributor

@aranas aranas commented Aug 13, 2024

this PR is a work in progress on the tutorial showcasing how autoemulate can be embedded into an end-to-end cardiac modelling pipeline including sensitivity analysis.

This tutorial still requires some improvements

  • add explanations to every step
  • add plot that showcases value of autoemulate in the context of sensitivity analysis more specifically

@aranas aranas self-assigned this Aug 13, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

blackfmt

[blackfmt] reported by reviewdog 🐶

fig, ax = plt.subplots(ncols=2, figsize = (10, 5))


[blackfmt] reported by reviewdog 🐶

ax[0].plot(self.res.t, self.res.y[2*i,:] , 'r', alpha=0.1 + (1.-i/self.ncomp) *0.9)
ax[1].plot(self.res.t, self.res.y[2*i+1,:] , 'r', alpha=0.1 + (1.-i/self.ncomp) *0.9)
ax[0].set_title('Pressure')
ax[1].set_title('Flow rate')
ax[0].set_xlabel('Time (s)')
ax[1].set_xlabel('Time (s)')
ax[0].set_ylabel('mmHg')
ax[1].set_ylabel('$ml\cdot s^{-1}$')
return (fig, ax)

Copy link
Contributor

github-actions bot commented Aug 13, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  .
  sensitivity_analysis.py 1-304
  autoemulate
  metrics.py
  autoemulate/emulators
  gaussian_process.py
  autoemulate/simulations
  flow_functions.py 1-162
Project Total  

This report was generated by python-coverage-comment-action

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 193 lines in your changes missing coverage. Please review.

Project coverage is 89.41%. Comparing base (005b3c0) to head (a6f7795).

Files with missing lines Patch % Lines
sensitivity_analysis.py 0.00% 108 Missing ⚠️
autoemulate/simulations/flow_functions.py 0.00% 85 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
- Coverage   94.17%   89.41%   -4.77%     
==========================================
  Files          62       64       +2     
  Lines        3606     3798     +192     
==========================================
  Hits         3396     3396              
- Misses        210      402     +192     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aranas
Copy link
Contributor Author

aranas commented Aug 13, 2024

@MaxBalmus I managed to transfer our original commits into the autoemulate repo, I think it makes more sense to work here directly, so that Martin can chip in where appropriate.
for example, I have now transferred Max's custom flow functions into the simulations file, but I realized that the other files contain functions, not classes @mastoffel, is this something we should adapt for consistency?

@mastoffel
Copy link
Collaborator

@MaxBalmus I managed to transfer our original commits into the autoemulate repo, I think it makes more sense to work here directly, so that Martin can chip in where appropriate. for example, I have now transferred Max's custom flow functions into the simulations file, but I realized that the other files contain functions, not classes @mastoffel, is this something we should adapt for consistency?

@MaxBalmus @aranas: This is great! It would be best to make the simulation a function to be consistent with the other simulations eventually, but this isn't urgent. Let me know when I should have a look at it!


# Use custom names if provided, else default to "x1", "x2", etc.
parameter_names = (
problem["names"] if problem is not None else [f"x{i+1}" for i in range(len(next(iter(results.values()))["S1"]))]
Copy link
Contributor

Choose a reason for hiding this comment

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

[blackfmt] reported by reviewdog 🐶

Suggested change
problem["names"] if problem is not None else [f"x{i+1}" for i in range(len(next(iter(results.values()))["S1"]))]
problem["names"]
if problem is not None
else [f"x{i+1}" for i in range(len(next(iter(results.values()))["S1"]))]

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should resolve #279

discrepancy = np.atleast_1d(discrepancy)
n_obs = len(obs_mean)
rank = min(max(rank, 0), n_obs - 1)
# Vs represents the total variance associated with the observations, predictions, and potential discrepancies.
Copy link
Contributor

Choose a reason for hiding this comment

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

[blackfmt] reported by reviewdog 🐶

Suggested change
# Vs represents the total variance associated with the observations, predictions, and potential discrepancies.
# Vs represents the total variance associated with the observations, predictions, and potential discrepancies.

Copy link
Member

@radka-j radka-j left a comment

Choose a reason for hiding this comment

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

I made some suggestions, mostly around simplifying the notebook to make it just focus on autoemulate capability and nothing. In brief:

  • the intro should just say the purpose of this tutorial is to demonstrate how autoemulate can be used to do sensitivity analysis (and maybe also to demonstrate how it works on a more complex example)
  • I would also remove the end part which uses the SALib package to do some further sensitivity analyses

@@ -0,0 +1,304 @@
import matplotlib.pyplot as plt
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be removed? It's a duplicate of a same named file in the autoemulate directory (where it should be).

"cell_type": "markdown",
"metadata": {},
"source": [
"## Context\n",
Copy link
Member

Choose a reason for hiding this comment

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

I'd reorder and slightly reword the intro section. Specifically, I'd remove context (people who are reading the autoemulate docs already know this) and start with purpose of the tutorial instead i.e., explain why someone reading the docs should read this page. Some of the context text explain gin the cardio side might need to be moved elsewhere.

I made some rough edit suggestions to this end but they are not necessarily comprehensive.

"## Purpose\n",
"In this tutorial, we will demonstrate the application of `AutoEmulate` within a cardiac modeling pipeline, specifically focusing on the reconstruction of critical heart parameters using synthetic data. The goal is to demonstrate how these measures—such as arterial systemic pressure, and flow rates —can be leveraged to estimate a range of model parameters related to cardiac mechanics and cardiovascular hemodynamics. \n",
"\n",
"By the end of this tutorial, participants will have a clear understanding of how to use emulation to facilitate the real-time simulation of complex cardiac dynamics on a local CPU.\n",
Copy link
Member

Choose a reason for hiding this comment

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

Using the word participants makes this sound like it's meant for an external event. I'd drop that throughout and instead use the style of the rest of the documentation ("we will do this...").

Suggested change
"By the end of this tutorial, participants will have a clear understanding of how to use emulation to facilitate the real-time simulation of complex cardiac dynamics on a local CPU.\n",

"\n",
"By the end of this tutorial, participants will have a clear understanding of how to use emulation to facilitate the real-time simulation of complex cardiac dynamics on a local CPU.\n",
"\n",
"Participants will learn how to explore the relationship between underlying cardiac health parameters and non-invasive measures using synthetic data, with a focus on how these insights can improve understanding and estimation of key cardiovascular dynamics."
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, it feels like the purpose here is to learn about autoemulate, not cardiac health

Suggested change
"Participants will learn how to explore the relationship between underlying cardiac health parameters and non-invasive measures using synthetic data, with a focus on how these insights can improve understanding and estimation of key cardiovascular dynamics."

"metadata": {},
"outputs": [],
"source": [
"import os\n",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is used throughout

Suggested change
"import os\n",

"1. setting up a simulation : The system simulated is a tube with an input flow rate at any given time. The tube is divided to 10 compartments which allows for the study of the pressure and flow rate at various points in the tube. \n",
"2. Running the simulation for 60 sets of parameters sampled from the parameter space. \n",
"3. applying Autoemulate to find the best emulator fot this simulation \n",
"5. assessing model accuracy, performing sensitivity analysis, and comparing computational resources for different modeling approaches. "
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"5. assessing model accuracy, performing sensitivity analysis, and comparing computational resources for different modeling approaches. "
"5. Assessing model accuracy and performing sensitivity analysis. "

]
},
{
"cell_type": "code",
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by everything that follows from here onwards. The above cell demonstrates how to do sensitivity analysis with AutoEmulate but now it seems that we've reverted to using the SALib package to do some further custom analysis? I'd remove the rest of this section and if there is anything here that we think that autoemulate should capture we can add it to an issue to tackle separately.

"cell_type": "markdown",
"metadata": {},
"source": [
"# Scaling Sensitivity Analysis with Emulation"
Copy link
Member

Choose a reason for hiding this comment

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

This could either say "scaling sensitivity analysis with autoemulate" or I'd leave just as is given the user is coming across this in the autoemulate docs

Suggested change
"# Scaling Sensitivity Analysis with Emulation"
"# Scaling Sensitivity Analysis"

"metadata": {},
"source": [
"## Context\n",
"In the field of cardiovascular modeling, capturing the dynamics of blood flow and the associated pressures and volumes within the vascular system is crucial for understanding heart function and disease. Physics-based models that accurately represent these dynamics often require significant computational resources, making them challenging to apply in large-scale or real-time scenarios. Emulation techniques provide a way to achieve high-fidelity simulations of the cardiovascular system, allowing for efficient and accurate analysis of key hemodynamic parameters."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"In the field of cardiovascular modeling, capturing the dynamics of blood flow and the associated pressures and volumes within the vascular system is crucial for understanding heart function and disease. Physics-based models that accurately represent these dynamics often require significant computational resources, making them challenging to apply in large-scale or real-time scenarios. Emulation techniques provide a way to achieve high-fidelity simulations of the cardiovascular system, allowing for efficient and accurate analysis of key hemodynamic parameters."

"metadata": {},
"source": [
"## Purpose\n",
"In this tutorial, we will demonstrate the application of `AutoEmulate` within a cardiac modeling pipeline, specifically focusing on the reconstruction of critical heart parameters using synthetic data. The goal is to demonstrate how these measures—such as arterial systemic pressure, and flow rates —can be leveraged to estimate a range of model parameters related to cardiac mechanics and cardiovascular hemodynamics. \n",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"In this tutorial, we will demonstrate the application of `AutoEmulate` within a cardiac modeling pipeline, specifically focusing on the reconstruction of critical heart parameters using synthetic data. The goal is to demonstrate how these measures—such as arterial systemic pressure, and flow rates —can be leveraged to estimate a range of model parameters related to cardiac mechanics and cardiovascular hemodynamics. \n",
"In this tutorial, we will demonstrate the application of `AutoEmulate` within a cardiac modeling pipeline, specifically focusing on the reconstruction of critical heart parameters using synthetic data. \n",

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.

6 participants