-
Notifications
You must be signed in to change notification settings - Fork 15
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
ENH: Add rms volumetrics wrapper #704
ENH: Add rms volumetrics wrapper #704
Conversation
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.
This is an important first step in establishing how we could improve the general pattern of how users interact with dataio in the future so I think we should exert a lot of consideration into how that looks.
I left some comments for your consideration; I think this could be a good case for not using a dataclass since it is not really encapsulating its input arguments to be passed around, and half of the initial state is gathered from RMS as well
a6a1e44
to
e5387c7
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.
It's looking good, and please forgive me for being extra rigorous with this PR, but I think we should tread carefully with establishing new API patterns. I also think we should consider whether or not we want to encapsulate the export method into a class which is in a way adding an extra step -- even if it's still in a sense a one-liner
416ad20
to
8caddef
Compare
6943a7b
to
2a113c7
Compare
2a113c7
to
1eac24f
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.
I think this is just about good to go. A few leftover debugging things that need to be removed and some merge resolution duplication in conf.py, as well as some other comments for your consideration. Thanks very much for taking consideration of previous comments.
import warnings | ||
from typing import TYPE_CHECKING, Any, Dict, Optional | ||
|
||
# mypy: ignore-errors |
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.
Do we want this at the top-level? I believe it nullifies checking the entire file rather than the few lines where it should be applied
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.
There are multiple and annoying issues with mypy checks and pylance in vscode, which are contradicting each other; but found a workaround
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 work, and well documented 😃 I look forward to informing users of this, it will be a large simplification for them 🚀 while enabling us to do adjustments in the background in the future.
I don't have much to comment on here now, it LGTM 🙂 only thing I would like to avoid exposing the arguments that is marked as currently NOT implemented
until we plan to actually do it.
2e5cb10
to
a2ecdda
Compare
Proposal to add a one-liner for the volumetrics job in RMS
I choose to call a function; the alternative is to call a class with public methods. Not sure what is best.Chose now this pattern:RmsVolumetricsExport(...).export()
Revert to function:
from fmu.dataio.export.rms import export_rms_volumetrics
Will resolve #698
The tests are mocked and not optimal; see a need here to possibly do integration tests with RMS when that is possible.