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

Clarify gen data docs #6984

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Clarify gen data docs #6984

merged 2 commits into from
Jan 24, 2024

Conversation

yngve-sk
Copy link
Contributor

@yngve-sk yngve-sk commented Jan 22, 2024

Goal: Describe less ambiguously what a GEN_DATA result file must contain, why it is specified and how it is used. Also remove deprecated INPUT_FORMAT doc

@yngve-sk yngve-sk force-pushed the clarify-gen-data-docs branch 2 times, most recently from 40cfda6 to 7685445 Compare January 22, 2024 11:09
@yngve-sk yngve-sk self-assigned this Jan 22, 2024
@yngve-sk yngve-sk force-pushed the clarify-gen-data-docs branch 2 times, most recently from e7986dd to 1b1bf78 Compare January 22, 2024 11:26
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0465891) 84.37% compared to head (1b1bf78) 84.39%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6984      +/-   ##
==========================================
+ Coverage   84.37%   84.39%   +0.02%     
==========================================
  Files         367      367              
  Lines       21882    21882              
  Branches      900      900              
==========================================
+ Hits        18462    18468       +6     
+ Misses       3126     3120       -6     
  Partials      294      294              

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

@yngve-sk yngve-sk force-pushed the clarify-gen-data-docs branch from 1b1bf78 to 72908ee Compare January 22, 2024 11:59
@jonathan-eq jonathan-eq self-requested a review January 22, 2024 12:11
Copy link
Contributor

@jonathan-eq jonathan-eq left a comment

Choose a reason for hiding this comment

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

Great improvement! It was more clearly defined now.

Copy link
Contributor

@larsevj larsevj left a comment

Choose a reason for hiding this comment

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

One minor suggestion, otherwise looks good.

:ref:`GENERAL_OBSERVATIONs <general_observation>`. It declares
text files which have been generated by the forward model, which will be
loaded by ert when loading general observations.
These text files naming scheme may be arbitrary. The contents of these result files are always:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change: files -> files', in "These text files naming scheme may be arbitrary.".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing


The GEN_DATA keyword has several options, each of them required:

* RESULT_FILE - This is the name of the file generated by the forward
model and read by ERT. This filename _must_ have a %d as part of the
name, that %d will be replaced by report step when loading.
* INPUT_FORMAT - The format of the file written by the forward model
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the INPUT_FORMAT deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is, no longer in use

@yngve-sk yngve-sk force-pushed the clarify-gen-data-docs branch from 72908ee to fa29a5f Compare January 22, 2024 13:22
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

Thanks for updating the documentation, always welcomed to see! I just have some minor comments.

docs/reference/configuration/keywords.rst Outdated Show resolved Hide resolved
docs/reference/configuration/keywords.rst Outdated Show resolved Hide resolved
docs/reference/configuration/keywords.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

LGTM!

@yngve-sk yngve-sk merged commit 05f26d7 into equinor:main Jan 24, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants