History matching refine model #423
Conversation
…I will return to this
… the base class of the simulator
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #423 +/- ##
===========================================
- Coverage 90.17% 79.89% -10.28%
===========================================
Files 97 100 +3
Lines 5994 6915 +921
===========================================
+ Hits 5405 5525 +120
- Misses 589 1390 +801 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
marjanfamili
requested review from
cisprague,
radka-j,
sgreenbury and
ContiPaolo
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "#### 3 - Wrapping your Simulator in AutoEmulate Simulator Base Class\n", |
There was a problem hiding this comment.
To make this as straightforward for the user as possible, we could move the sample_inputs() method implementation from Naghavi to the Base Simulator. A user could always choose to override it but in most instances we want to do LatinHypercube sampling so it seems like a good default for all simulators to inherit.
If we do that, then we could simplify this section and simply say, the user has to subclass the Simulator class and implement the sample_forward method (I wouldn't even worry about mentioning _init_ here). I think that makes it really accessible.
There was a problem hiding this comment.
This sounds very reasonable to me, I will wait for more comments on this, if none, I will apply these changes
There was a problem hiding this comment.
Agreed - this sounds great to have as a default to simplify subclassing and would not be unexpected.
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "#### 9 - use the interactive dashboard to inspect the results of history matching " |
There was a problem hiding this comment.
The interactive dashboard doesn't display - is this something that we can fix?
If not, should we maybe comment out the code cell (unless there's a another way not to execute the code block) and just add a sentence above it that tells the user something like - if you are running this notebook interactively, you can view an interactive dashboard by uncommenting and running the below code. And/or could we maybe add a screenshot of it?
There was a problem hiding this comment.
Is there an error ? it does display for me locally with no issues
There was a problem hiding this comment.
I like the idea of a screenshot for the static version and uncommenting (or a global constant at the top of the notebook?) to enable the dashboard when running locally. I also tried running locally and the dashboard displays for me.
Co-authored-by: Radka Jersakova <[email protected]>
Co-authored-by: Radka Jersakova <[email protected]>
Co-authored-by: Radka Jersakova <[email protected]>
Co-authored-by: Radka Jersakova <[email protected]>
| output_variables if output_variables is not None else [] | ||
| ) | ||
| self._output_names = [] # Will be populated after first simulation | ||
| self._has_sample_forward = False |
There was a problem hiding this comment.
Is _has_sample_forward used anywhere?
docs/tutorials/01_start.ipynb
Outdated
| "\n", | ||
| "model = em.get_model(best_model['model'])\n", | ||
| "pred_mean, pred_std = _predict_with_optional_std(model, X)\n", | ||
| "pred_mean, pred_std = best_model.predict(X, return_std=True)\n", |
There was a problem hiding this comment.
Should this tutorial also used the HistoryMatcher class?
There was a problem hiding this comment.
yes it should , but should I just remove it from that tutorial ? is this an advance feature for 01_start ?
There was a problem hiding this comment.
good point, yes lets just remove it from the 01_start tutorial
| else: | ||
| # Run actual simulation | ||
| outputs = self.simulator.sample_forward(params) | ||
| if outputs is None: |
There was a problem hiding this comment.
Should a user warning be raised here?
…data/naghavi_model_parameters
misc/workflow.png
Outdated
There was a problem hiding this comment.
I really like this workflow graphic!
Having looked at this visualisation (which is very basic), I wonder whether it's worth distinguishing which bits of the pipeline are on the user (e.g., creating samples or providing data) whereas what the package does for them (e.g., select models). This could be done by using different shading for example.
There was a problem hiding this comment.
Typo, line 10 - "AutoEmulate" instead of "Autoemulate"
There was a problem hiding this comment.
Typo, line 12 - "Sensitivity Analysis"
There was a problem hiding this comment.
Perhaps it would be worth having the imports for the notebook all in a single cell after the ModularCirc pip install?
There was a problem hiding this comment.
Discussed with @marjanfamili and @radka-j - in this case it would be better to have each section self-contained to make usage clear.
| @@ -666,7 +666,8 @@ | |||
| } | |||
There was a problem hiding this comment.
Line #2. em.evaluate(gp)
I liked having best_model(instead of specifying manually "GaussianProcess") as it showcases that AE automatically selects the best model
Reply via ReviewNB
There was a problem hiding this comment.
Nice job overall!
For after the MVP:
One potential area for improvement is performance: there may be opportunities to replace some for loops with vectorized operations using NumPy or PyTorch, which could speed things up a bit.
It looks like the current use of a dictionary helps keep axis names associated with the data, which is great for clarity. But, we might consider whether using a single tensor for the data, alongside a separate list of axis name strings, could make things more efficient, especially if this allows for more straightforward vectorized operations.
| rank: Rank for history matching | ||
| """ | ||
| self.simulator = simulator | ||
| self.observations = observations |
There was a problem hiding this comment.
What would this look like? {'var0': (0.0, 0.0), ..., 'varn': (0.0, 0.0)}? Maybe it would be more efficient to have the keys as a separate list of strings, corresponding the the axes of a tensor? If we have the observations as a single tensor, then vectorized operations might give us a speed up.
… more fearures added, also removed show NROY button for the plots that is not rlevant for
…or all plots, modified 01_start and removed history matching, its an advance feature and not needed in the start tutorial, updated history_matching itself to handle none outputs and failed attempts as it was interrupting the history matching waves. added more information to the tqdm print , to show more information about how many NROY points , and failed attempts
📝 Description
This pull request introduces
NaghaviSimulatorHistoryMatcherHistoryMatchingDashboard👩💻 Usage
07_AE_workflow.ipynb🆘 Help Needed
autoemulatetest. maybe a test in thesimulationsfolder🔗 Related Issue
#294 #412
🛠 Type of Change
✅ Checklist
🖼️ Screenshots (if applicable)
💬 Additional Notes
Reviewers:
👀 Pay special attention to: