maximum number of 8 characters for fault names not enforced
Closed this issue · 15 comments
If a user specifies more than 8 character for a fault then flow uses all characters to construct the fault name while other simulators seems to disregard the characters after the 8th one. This leads to problem for models assuming the behavior of the other simulators and use just the first 8 characters when referring to the fault e.g. in MULTFLT. I have just seen "ABCE_04h_reduced_height" being used defining the fault name and later referring to it with "ABCE_04h"
To add to the confusion our error is "Key not found" without any additional information. 😳
So the question is: Should we
- Force users to use only 8 characters (and break their models)?
- Use only the first 8 characters as the fault name (as other simulators)?
In both cases we need to fix all keywords that take the name of a fault.
Will start fixing the error message...
My preference is to allow more than eight characters, but it seems that is not on the menu :-) from the two options I prefer the latter, chopping off the excessive characters, but allowing more than eight.
Is not this how almost any string is treated already, for example well names? Or do we accept and use arbitrary long names there?
Are there any strings, other than in the INCLUDE keyword and others that take paths, that we should NOT chop down to 8?
@alfbr Everything is on the menu. I just only listed the ones that would make it easier to either use the model or to fix it.
Our manual states clearly that fault names have a maximum of 8 characters. We just do not act on it in any way
I have no idea how hard it would be to implement, but I think the most user-friendly option here would be to permit long names and first try an exact match, but fall back to initial 8-character prefix matching if that fails. This will of course have a runtime cost so it's not something that should be added without careful measurement. On the other hand, this approach would probably also address issues like #1856—nominally handled in #1857—which may or may not recur in other models.
What about just looking at the first 8 characters in the map comparison operator?
What about just looking at the first 8 characters in the map comparison operator?
That will do the right thing in the common case but it will fail randomly if we happen to get a model for which the initial 8-character prefix of two distinct faults is the same—e.g.,FAULT-SOUTH-1 and FAULT-SOUTH-2.
That will do the right thing in the common case but it will fail randomly if we happen to get a model for which the initial 8-character prefix of two distinct faults is the same—e.g.,
FAULT-SOUTH-1andFAULT-SOUTH-2.
Should we not treat those two as the same fault? Just as "ABCE_04h_reduced_height" and "ABCE_04h"?
I think this is overengineering.
Not sure which of the ideas you think is overengineering, and which you favour?
I think we should follow the behavior of the commercial simulator, issue a warning stating that the fault name is greater than the allowable eight characters and has been truncated. For example:
Warning: Unsupported keywords or keyword items:
FAULTS: invalid fault name - ABCE_04h_reduced_height
In file: MOD01-TEMP.DATA, line 664
FAULTS(FLTNAME) greater than 8 characters, truncated to: ABCE_04h - will continue
Otherwise it just adds inconsistency between decks. Engineers know the eight character rule, but unfortunately our geoscience friends always seem to forget it .
We should definitely not consider supporting FAULTS(FLTNAME) greater than 8 characters.
What about just looking at the first 8 characters in the map comparison operator?
As this is an unordered_map internally we need to do this both for the predicate and the hash functions. Seems like a nice approach that we combine with a warning if needed.
While at it I would also get rid off the template parameter for the key type of OrderedMap as this would simply things. Currently only std::string is used anyway. Does anybody envision another type to be used? Seems unlikely, right?
@blattms: Is this still a problem following #3065 and OPM/opm-simulators#3959?
thanks for the hint. Not an issue anymore, sorry for not closing earlier.