-
Notifications
You must be signed in to change notification settings - Fork 110
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
Rename job.py to fm_step.py and Job to ForwardModelStep #9022
Conversation
ec730d9
to
d2092cf
Compare
@@ -76,7 +76,7 @@ def killed_by_oom(pids: Sequence[int]) -> bool: | |||
return False | |||
|
|||
|
|||
class Job: | |||
class FMstep: |
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.
Maybe a nitpick, but I think the S should be big too?
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.
It probably should. Potentially go all the way and write it as ForwardModelStep? That object name is actually not in use anywhere else in the codebase (although we do use it as a prefix for some other object names)
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.
ForwardModelStep sounds better to me
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.
Fixed.
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.
👍
d2092cf
to
65f58d3
Compare
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.
LGTM. Final nitpick; would consider renaming the file fm_step.py to forward_model_step.py for consistency, but this is a good change regardless
Plus follow-up in test code and filenames
65f58d3
to
d85caf2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9022 +/- ##
==========================================
+ Coverage 90.87% 90.96% +0.09%
==========================================
Files 352 352
Lines 21712 21712
==========================================
+ Hits 19730 19751 +21
+ Misses 1982 1961 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Issue
Resolves eye itch
Approach
nvim
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"'
)When applicable