Update Simulator base class #520
Conversation
Co-authored-by: Sam Greenbury <[email protected]> Co-authored-by: Edwin <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #520 +/- ##
==========================================
- Coverage 81.90% 76.55% -5.36%
==========================================
Files 107 113 +6
Lines 7571 8270 +699
==========================================
+ Hits 6201 6331 +130
- Misses 1370 1939 +569 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Co-authored-by: Sam Greenbury <[email protected]>
There was a problem hiding this comment.
I think the new cell outputs might need to be included here given the current way the docs are published without rerunning with jupyterbook.
There was a problem hiding this comment.
I removed them because when I run the notebook and committed the output I got a git error complaining about the commit size.
There was a problem hiding this comment.
Great point - as discussed opened #538 to look at a solution for this.
| # TODO: have option to set dtype and ensure consistency throughout codebase? | ||
| # added here as method was returning float64 and elsewhere had tensors of | ||
| # float32 and this caused issues | ||
| return torch.tensor(sample_array, dtype=torch.float32) |
There was a problem hiding this comment.
I think this would be great to potentially have a default constant somewhere (e.g. DEFAULT_DTYPE: torch.dtype = torch.float32) that we can import to use such as here. We might also consider adding API to e.g. the base simulator/emulators so that dtype= can be provided. Perhaps this would be good to handle in a new issue?
There was a problem hiding this comment.
Great idea, I created issue #537 for this
| class SimulatorMetadata: | ||
| """ | ||
| Simulation metadata. | ||
|
|
||
| NOTE: the output metadata is going to get more complex. e.g.: | ||
| - indicate if output is temporal | ||
| - indicate if output is spatial | ||
| - ... |
There was a problem hiding this comment.
Following on from discussion/question in the PR - it seems like this separate class might be currently not providing much use and could be brought into the base simulator.
The comment regarding the complexity that will increase with different data types is important and perhaps a refactor around a common metadata into a base class and data-type specific metadata (e.g. tabular data or spatio-temporal data) would be useful at that point.
Perhaps it would be worth opening an issue specifically for the spatio-temporal simulator as an enhancement? (we currently have #459 that I think is more focussed on the emulator aspect)
There was a problem hiding this comment.
This class has been removed for now. I think the enhancement could be done as part of #472
Co-authored-by: Sam Greenbury <[email protected]>
3 participants
Closes #414
Overview
This PR:
output_variablesargument as input toSimulatoras it was specific to theNaghaviSimulatorimplementation rather than a generic simulator argumentI left updates to
NaghaviSimulatorto be tackled as a separate PR (see discussion in #492).Questions
sample_inputsmethod started retuning tensors of float64 whereas everywhere else in the code base the tensors are float32 (this is the torch default). I have no idea why this happened and it made me think we might want to enforce consistency of types across the codebase. This could be us picking a type to stick to or enforcing a type based on inputs. Should we open an issue for that?