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

Document everest forward models #8940

Merged

Conversation

frode-aarstad
Copy link
Contributor

Issue
Resolves #8835

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@frode-aarstad frode-aarstad self-assigned this Oct 10, 2024
@frode-aarstad frode-aarstad marked this pull request as draft October 10, 2024 12:49
@frode-aarstad frode-aarstad force-pushed the document-everest-forward-models branch from 24e50da to fd9aa5a Compare October 11, 2024 12:05
@frode-aarstad frode-aarstad marked this pull request as ready for review October 11, 2024 12:06
@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 45.61404% with 62 lines in your changes missing coverage. Please review.

Project coverage is 90.95%. Comparing base (ebc4d03) to head (7d844b8).
Report is 437 commits behind head on main.

Files with missing lines Patch % Lines
...t/shared/_doc_utils/forward_model_documentation.py 33.92% 37 Missing ⚠️
src/ert/shared/_doc_utils/everest_jobs.py 52.77% 17 Missing ⚠️
src/ert/shared/_doc_utils/__init__.py 57.89% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8940      +/-   ##
==========================================
- Coverage   91.00%   90.95%   -0.06%     
==========================================
  Files         349      352       +3     
  Lines       21641    21691      +50     
==========================================
+ Hits        19695    19729      +34     
- Misses       1946     1962      +16     
Flag Coverage Δ
cli-tests 39.11% <0.00%> (-0.08%) ⬇️
gui-tests 72.76% <0.00%> (-0.13%) ⬇️
performance-tests 49.60% <0.00%> (-0.12%) ⬇️
unit-tests 79.67% <45.61%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@frode-aarstad frode-aarstad force-pushed the document-everest-forward-models branch from fd9aa5a to b377530 Compare October 11, 2024 12:24
@yngve-sk yngve-sk self-requested a review October 15, 2024 07:00
@yngve-sk
Copy link
Contributor

yngve-sk commented Oct 15, 2024

Screenshot 2024-10-15 at 14 22 53
(left=docs on this PR, right=docs on main)

Overall looks good, and is much closer to having same format as ERT forward model steps.

I tried building both and comparing and some things should require another look:

  1. I think you added some docs for previously undocumented forward model steps, which is great, but the naming in the built docs is a bit different, for example fm_compute_economics, I guess there should be a more "displayable" name there.
  2. The front page of the Forward Model Jobs section is missing the introduction that can be seen on the screenshot on the right
  3. The Eclipse simulator docs seem to be missing, but it is also in reality missing from the Everest jobs? Or well, it is there through ERT's default installed forward model steps in site config, which complicates the docs thing a bit more.



@hookimpl
def get_forward_model_documentations():
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? I see there are similar entries in there from before, and wondering the same thing about them (just relevant, but outside scope of PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think the other functions in this interface provides what we want and I don't want to change existing behavior. Or what were you thinking about?

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be wrong here, but I think @hookimpl just means it is an implementation, so this empty function just provides an empty documentation, so it should be possible to remove it without any side effects (I think, needs to be double checked)


def get_documentation(self) -> Dict[str, Any]:
docs = self.hook.get_forward_model_documentations()
return docs[0] if docs else {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why [0] here instead of merging dicts? Maybe non-relevant edge case but what if there were other installed python packages containing forward models for everest (currently only everest-models though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

@frode-aarstad
Copy link
Contributor Author

Screenshot 2024-10-15 at 14 22 53 (left=docs on this PR, right=docs on main)

Overall looks good, and is much closer to having same format as ERT forward model steps.

I tried building both and comparing and some things should require another look:

1. I think you added some docs for previously undocumented forward model steps, which is great, but the naming in the built docs is a bit different, for example `fm_compute_economics`, I guess there should be a more "displayable" name there.

2. The front page of the `Forward Model Jobs` section is missing the introduction that can be seen on the screenshot on the right

3. The Eclipse simulator docs seem to be missing, but it is also in reality missing from the Everest jobs? Or well, it is there through ERT's default installed forward model steps in site config, which complicates the docs thing a bit more.
  1. I will add a 'full name' for the 'new' forward models
  2. I will add the introduction
  3. Not sure what to do with the Eclipse simulator docs. Do they belong here amongst the forward models? Any thoughts @oyvindeide

@frode-aarstad frode-aarstad force-pushed the document-everest-forward-models branch 2 times, most recently from 60a1ae7 to 5b9b929 Compare October 21, 2024 12:53
@yngve-sk
Copy link
Contributor

Almost there, ecl100 docs now also look exactly same as before. Did you add the "display names" commit yet? I'm still getting this:
Screenshot 2024-10-22 at 09 00 34

@frode-aarstad
Copy link
Contributor Author

Almost there, ecl100 docs now also look exactly same as before. Did you add the "display names" commit yet? I'm still getting this: Screenshot 2024-10-22 at 09 00 34

Its in the everest models PR equinor/everest-models#64

Copy link
Contributor

@yngve-sk yngve-sk left a comment

Choose a reason for hiding this comment

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

Nice job! LGTM

@frode-aarstad frode-aarstad force-pushed the document-everest-forward-models branch from 5b9b929 to 7d844b8 Compare October 22, 2024 10:32
@frode-aarstad frode-aarstad merged commit b17e439 into equinor:main Oct 22, 2024
56 checks passed
@frode-aarstad frode-aarstad deleted the document-everest-forward-models branch October 22, 2024 10:48
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.

Use plugin functionality to document forward models in Everest
3 participants