-
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
Limit the use of EverestConfig in function signatures in everest csv exporting #9163
Limit the use of EverestConfig in function signatures in everest csv exporting #9163
Conversation
c1ad78a
to
3b16502
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9163 +/- ##
==========================================
- Coverage 90.76% 90.74% -0.03%
==========================================
Files 351 351
Lines 21901 21903 +2
==========================================
- Hits 19879 19876 -3
- Misses 2022 2027 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
8c90bf0
to
b995073
Compare
b995073
to
2622c1b
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.
The refactor in itself looks good but I have some things I'm not totally sure about. For the validation you got rid of the direct everest config and this is nice, and using only batches
and keywords
from self
.
For the export_data
, if I understood correctly, this cannot be put within the export config, because the export config may be null even if you want to export, but if you make this a staticmethod of ExportConfig (or keep it in its original place). I also think if you make model.data_file
and .output_dir
explicit arguments you can drop having to reference an EverestConfig as is done through the self
reference there.
I'm not sure if I see why the export logic should be partially moved to the config? They still refer to some stuff under export.py
, and the pattern leans towards making it easy to suddenly get circular imports, so if they are to be moved to the configs I'd at least say do it fully and get rid the entire export.py
file, or keep it there. In ERT we do have some logic in the configs (maybe too much) but it sort of makes sense to keep the exporting logic in the export config. Would try to get it all into the ExportConfig and make the things that should run without an instance a @staticmethod
I see your point I will just move it back to |
5254b7b
to
9fbd447
Compare
9fbd447
to
5bf883f
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.
Nice! Behavior is the same and the logic is more clear 🚀
Issue
Resolves #9162
git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"'
)When applicable