generated from ucgmsim/template
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Updated Realisations #6
Open
lispandfound
wants to merge
15
commits into
main
Choose a base branch
from
realisation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lispandfound
requested review from
sungeunbae,
joelridden,
claudio525 and
AndrewRidden-Harper
October 16, 2024 03:25
sungeunbae
reviewed
Oct 16, 2024
sungeunbae
previously approved these changes
Oct 16, 2024
joelridden
reviewed
Oct 20, 2024
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.
Just a few comments, deptry issue is interesting
lispandfound
dismissed
sungeunbae’s stale review
October 21, 2024 21:43
The merge-base changed after approval.
AndrewRidden-Harper
approved these changes
Oct 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Realisations
The following PR updates the realisations for the new workflow. The API for realisations has changed since the last pull request for realisations. The new workflow realisations aim to be a well-documented, modular, and self-contained method for simulation configuration. The idea is that the scientific defaults version, container version, and realisation file should completely determine the simulation output up to hardware-level floating point mathematics differences.
Note
Only the
realisations.py
,test_realisation.py
, andschemas.py
files need to be reviewed. The defaults are otherwise as they are in #5 , and are there to allow the tests to pass.Using the Realisations Module
Reading and writing realisations involves using one of the classes to read and write realisation configurations. To retrieve the domain parameters of the realisation for example you would write something like
Realisations can load default values for some configuration sections using
read_from_realisation_or_defaults
Realisations have rudimentary value checking applied in the
schemas
module, which also defines the schema for each realisation section. For instance, here is the schema for the velocity model parameters.Adding a new realisation configuration is done by inheriting from the
RealisationConfiguration
class and then defining the key in the json file, the schema to validate against, and the values to store. Overriding theto_dict
method lets you change the JSON serialisation behaviour. Here is an example of that for the velocity model parameters.Some values for the realisation configuration are computed simply using values stored in the file. These values are now computed using Python properties rather than being stored in the realisation. For example the
nx
,ny
, andnz
values for the domain parameters are properties computed from the domain.Having these as properties means it is impossible to have the
nx
,ny
andnz
values be inconsistent with the domain.Testing
Inline with the discussions on testing earlier in the year, there is extensive testing for every line of the realisation and schema module. The tests are located in the
test_realisation.py
file. The tests ensure: