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

Localisation: Added option to specify GEN_OBS nodes in the form nodename:index in localisation config file #363

Closed
wants to merge 2 commits into from

Conversation

oddvarlia
Copy link
Contributor

@oddvarlia oddvarlia commented Oct 28, 2021

To be able to specify localisation were individual observations within an obs node of type GEN_OBS is used, it can be specified in the localisation config file in the same way as model parameters of type GEN_PARAM with nodename:index

To avoid problems mixing observation nodes of type GEN_OBS with notation: nodename and nodename:index a keyword is defined to specify if individual GEN_OBS observations is allowed to be specified or not. Since a GEN_OBS node specified in the ERT config file can be expanded into N individual observations, a GEN_OBS node is expanded into a list of N observation names: [ nodename:0, nodename:1, ....,nodename:N-1].
The user will then have to specify observations of type GEN_OBS in the form nodename:index.

But for some nodes of type GEN_OBS, like seismic data, the number N can be very large and for those cases it is best not to specify individual observations, hence, the form nodename is used, not expanded into individual observations.

@oddvarlia oddvarlia marked this pull request as draft October 29, 2021 15:20
@oddvarlia oddvarlia changed the title Added option to specify GEN_OBS nodes in the form nodename:index Localisation: Added option to specify GEN_OBS nodes in the form nodename:index in localisation config file Oct 29, 2021
@oddvarlia oddvarlia force-pushed the use_active_obs branch 3 times, most recently from fe66e39 to 2e081a1 Compare November 11, 2021 09:02
@oddvarlia oddvarlia force-pushed the use_active_obs branch 6 times, most recently from 360dbdf to a8199cf Compare November 23, 2021 13:54
@oddvarlia oddvarlia force-pushed the use_active_obs branch 2 times, most recently from 66aff11 to b3c5738 Compare November 29, 2021 23:20
@oddvarlia oddvarlia marked this pull request as ready for review November 29, 2021 23:23
@oddvarlia oddvarlia force-pushed the use_active_obs branch 9 times, most recently from cc01eb3 to fbaa8bc Compare December 3, 2021 18:28
.pre-commit-config.yaml Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
semeio/workflows/localisation/local_script_lib.py Outdated Show resolved Hide resolved
@oddvarlia oddvarlia force-pushed the use_active_obs branch 4 times, most recently from a6a1410 to 64d282e Compare January 12, 2022 10:33
@@ -853,3 +878,31 @@ def write_qc_parameter(
grid.write_grdecl(scaling_kw, file)
# Increase parameter number to define unique parameter name
cls.scaling_param_number = cls.scaling_param_number + 1


def get_obs_from_ert(ert, expand_gen_obs_max_size):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid the special casing of GEN_OBS?

Copy link
Contributor

Choose a reason for hiding this comment

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

If not we should add some testing of it

Copy link
Contributor Author

@oddvarlia oddvarlia Jan 17, 2022

Choose a reason for hiding this comment

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

The reason why I defined this 'expand_gen_obs_max_size' was to avoid having to expand a GEN_OBS node into the list of type nodename: index for index from 0 to size where the size sometimes can be very large (seismic data with 100.000's of values). The use of individual GEN_OBS observations is primarily meant for GEN_OBS nodes of moderate size. Examples I was thinking about was e.g geophysical data like gravimetry measurements or other cases where the number of observations in a GEN_OBS node is < 100. In this case it can be meaningful to setup localisations between individual observations and model parameters. For seismic data, the obvious way of handling that is to split the seismic data into a moderate number of groups (e.g one per fault block or segment) and define one GEN_OBS node for each of these and use these without referring to individual observations within the GEN_OBS node. Sometimes, like for isolated segments, it can be relevant to setup localisation where a GEN_OBS node for a segment is used together with the model parameter values related to the same segment. I thought at least that there might be unnecessary overhead to work with lists of 100.000's of observations in the localisation script and introduced this expand_gen_obs_max_size parameter to handle ERT models where there exists GEN_OBS nodes having large number of observations and GEN_OBS nodes having moderate number or small number of observations ( < expand_gen_obs_max_size). Do you have any better idea here?

if len(obs_index_list) > 0:
active_obs_list = obs_group.getActiveList(obs_node_name)
if len(obs_index_list) > 50:
debug_print(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still argue that this should be a documentation issue instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean that the user should set the number, hardcoded to 50, or that I should drop the debug print out here? I can of course write a sentence in the user doc, but I don't think the users care much about this as long as the output is not too large. Maybe I have misunderstood something?

@oddvarlia
Copy link
Contributor Author

This PR is waiting for some use cases before we merge it into main branch

@oddvarlia oddvarlia marked this pull request as draft January 21, 2022 10:14
@oddvarlia oddvarlia force-pushed the use_active_obs branch 2 times, most recently from cc0df89 to 459bca5 Compare January 25, 2022 19:10
@oddvarlia oddvarlia force-pushed the use_active_obs branch 2 times, most recently from 5eb9b80 to 241ac7e Compare March 28, 2022 10:41
@ManInFez
Copy link
Contributor

ManInFez commented Apr 1, 2022

Is this a draft or is it ready for review?

@oddvarlia oddvarlia force-pushed the use_active_obs branch 2 times, most recently from 105c6f3 to 432ad85 Compare May 13, 2022 08:45
This makes it possible to specify individual observations from an GEN_OBS node
in localisation.
Add tests to verify active param and active obs in ministeps
Updated integration tests with consistency check for active obs and param
Fix time and date format in testdata
Updated documentation of localisation using individual observations from GEN_OBS nodes.
@berland
Copy link
Collaborator

berland commented Jan 5, 2023

Closing due to no traction. Feel free to reopen whenever there is something new to add to this.

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.

Extend localisation with methods suitable for GEN_OBS type of observation nodes
4 participants