OPM / opm-common

Common components for OPM, in particular build system (cmake).
http://www.opm-project.org
GNU General Public License v3.0
30 stars 110 forks source link

GDFILE FMTOPT value is silently ignored and detects FORMATTED/UNFORMATTED based on file extension #2646

Open eivindjahren opened 2 years ago

eivindjahren commented 2 years ago

When importing grid via

GDFILE
  'MYFILE.FEGRID' UNFORMATTED /

although the user manual does not mention it, the UNFORMATTED is silently ignored, and the format of the file is detected from extension.

https://github.com/OPM/opm-common/blob/d7389f9b3ce2abcc86457bd65bcda6cf32e367ac/src/opm/parser/eclipse/EclipseState/Grid/GridDims.cpp#L151-L154

https://github.com/OPM/opm-common/blob/84c0f73a3360f4336b1cbdb7ffbdbf377a75ae4f/src/opm/parser/eclipse/EclipseState/Grid/EclipseGrid.cpp#L532-L545

joakim-hove commented 2 years ago

Ehhh yes - that seems to be correct. Unless this is really biting someone I do not see it as a runner up for this weeks top priority task, but hey a bug is a bug.

OPMUSER commented 2 years ago

Bug or feature is in the eyes of the beholder, as they say. I think this is a feature rather than a bug as it saves the user a bit of typing.

I can add the following text to the manual instead:

If the variable FMTOPT is omitted then the default is for binary file input for the commercial simulator; whereas, OPM Flow derives FMTOPT from the file extension (*.EGRID or *.FEGRID), making FMTOPT superfluous.

@eivindjahren if you are happy with that then close the ticket.

eivindjahren commented 2 years ago

@OPMUSER If going down that rute, I think a warning for when the second value is included in the file could be good also. The bug is not necessarily the part where it detects the format from the file, but that it does so silently. For instance, the current detection method thinks grid.fegrid (lower case) is an unformatted file so

GDFILE
 'grid.fegrid' F /

will fail to include the file as it is formatted, but detection thinks it is unformatted. Debugging from the user side requires some trial and error.

OPMUSER commented 2 years ago

Rather than a warning we should fix the case issue instead, so that it works as intended.