Draft: Partial deck parsing #487
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #487 +/- ##
==========================================
+ Coverage 95.18% 95.21% +0.03%
==========================================
Files 33 33
Lines 4463 4494 +31
==========================================
+ Hits 4248 4279 +31
Misses 215 215 ☔ View full report in Codecov by Sentry. |
| @@ -83,7 +83,7 @@ def get_path(self) -> Path: | |||
| """Return the full path to the directory with the .DATA file""" | |||
| return Path(self._eclbase).absolute().parent | |||
|
|
|||
| def get_deck(self) -> "opm.libopmcommon_python.Deck": | |||
| def get_deck(self, sections: list = []) -> "opm.libopmcommon_python.Deck": | |||
There was a problem hiding this comment.
https://docs.python-guide.org/writing/gotchas/
use sections: list = None and then set it to the empty list later if None.
There was a problem hiding this comment.
considered to use None, but used an empty list as that is what is the default in opm.io.Parser.parse and tried to keep it more or less consistent. Open to change it.
There was a problem hiding this comment.
It is a bug, you need to change it :)
There will be situations where you think you have an empty list, but then it isn't.
There was a problem hiding this comment.
Not sure if I understand you correctly.
The case here is:
If you give the opm parser an empty section list (which is the default), it returns the full deck.
If your opinion is that if you give an empty section list the expected returned value would be empty and that it might be confusing, I don't disagree with that. Also why I considered None in the first place. But I wouldn't call that a bug considering it was intended to keep the same format and behavior as opm.
So basically my question is: is what described above the "bug" or is it something else? I have no big issue with switching the default to None, but would like to know exactly what you mean as "the bug" to fix is 😉
There was a problem hiding this comment.
There was a problem hiding this comment.
Pylint should also give you
https://pylint.pycqa.org/en/latest/user_guide/messages/warning/dangerous-default-value.html
There was a problem hiding this comment.
It bit me hard once: equinor/fmu-ensemble@f109b6e
| deck = resdatafiles.get_deck( | ||
| sections=[opm.io.eclSectionType.RUNSPEC, opm.io.eclSectionType.PROPS] | ||
| ) | ||
| except ( |
There was a problem hiding this comment.
You may want to move this except to inside get_deck()?
There was a problem hiding this comment.
Will take a look at it, probably a good idea.
There was a problem hiding this comment.
seems like an issue in this case will be that we cannot run it with opm < 2024.04 (which doesn't exist yet), as opm.io.eclSectionType.RUNSPEC isn't valid (without complicating the code outside get_deck).
An option is to instead have a list of strings as the get_deck() input and then map them to opm.io.eclSectionType inside get_deck(). Might also make it easier for most users.
E.g. a syntax like this get_deck(sections=["RUNSPEC", "PROPS"])
| @@ -75,6 +75,15 @@ def test_filedescriptors(): | |||
| assert len(list(fd_dir.glob("*"))) == pre_fd_count | |||
| assert resdatafiles._rftfile is None | |||
|
|
|||
| deck = resdatafiles.get_deck(sections=[opm.io.eclSectionType.PROPS]) | |||
There was a problem hiding this comment.
These new lines should probably be in a new test function, it is not related to testing of file descriptors.
Quick attempt to resolve #260
A small question is how to handle the caching of decks in
ResdataFiles. In this draft I cached only if requesting the full deck, and returned the cached even if only sections are requested. That means that the returned value if requesting sections might vary dependent on whether or not a full deck has previously been requested. Considering this is mainly implemented for speed increase I think it could be ok, but open to discuss that.