Smspec cmp + correct padding of ecl_kw string keywords #146
Conversation
joakim-hove
force-pushed
the
smspec-cmp
branch
9 times, most recently
from
b05cb72
to
484cc4a
Compare
joakim-hove
changed the title
| ecl_string[i] = ' '; | ||
|
|
||
| ecl_string[type_len] = '\0'; | ||
| } |
There was a problem hiding this comment.
I think the following is more readable:
for (int i = 0; i < type_len; ++i) {
ecl_string[i] = (i < input_len) ? s[i] : ' ';
}And also, you don't need the { and the } guards here.
lib/ecl/smspec_node.c
Outdated
| if (node2_early) { | ||
| if (!node1_early) | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
I think the following is more readable:
if (node1_early && !node2_early)
return -1;
if (!node1_early && node2_early)
return 1;
lib/ecl/tests/ecl_smspec_node.c
Outdated
| smspec_node_free( well_node1_2 ); | ||
| smspec_node_free( well_node2_2 ); | ||
| //smspec_node_free( well_node_dummy ); | ||
| } |
There was a problem hiding this comment.
There are a couple of out-commented lines here, should they be removed? (Perhaps they are removed in a later commit.)
| return True | ||
| else: | ||
| return False | ||
|
|
There was a problem hiding this comment.
this should be
return self.cmp(other) < 0
| if cmp > 0: | ||
| return True | ||
| else: | ||
| return False |
| if cmp == 0: | ||
| return True | ||
| else: | ||
| return False |
python/tests/ecl/test_sum.py
Outdated
| self.assertEqual( node3.getNum( ) , 1 ) | ||
| self.assertTrue( node3.isTotal( )) | ||
|
|
||
| self.assertTrue( node1 < node3 ) |
There was a problem hiding this comment.
I would prefer self.assertLess(node1, node3)
python/tests/ecl/test_sum.py
Outdated
| self.assertTrue( node1 < node3 ) | ||
| self.assertTrue( node2 > node3 ) | ||
| self.assertTrue( node1 == node1 ) | ||
| self.assertFalse( node1 == node2 ) |
There was a problem hiding this comment.
assertEqual and assertNotEqual.
lib/ecl/smspec_node.c
Outdated
| if (cmp == 0) | ||
| return true; | ||
| else | ||
| return false; |
There was a problem hiding this comment.
Why not
return smspec_node_cmp(node1, node2) == 0;?
lib/ecl/tests/ecl_smspec.c
Outdated
| @@ -23,6 +23,17 @@ | |||
|
|
|||
| #include <ert/ecl/ecl_smspec.h> | |||
|
|
|||
| void test_sort( ecl_smspec_type * smspec ) | |||
| { | |||
| ecl_smspec_sort( smspec ); | |||
There was a problem hiding this comment.
This is my paranoia speaking, but could you add
test_assert_true(ecl_smspec_num_nodes(smspec) == 10);for 10 being the correct size of smspec?
There was a problem hiding this comment.
I quite frankly do not know the number of nodes, but I have added a check verifying that is not changed by the sort.
|
Thank you for the review - of course I will consider your comments. |
|
OK - I have addressed all the comments except: #146 (review) - where I really thought my initial version was more readable. Will merge when/if the rebased tests are green. |
Due to the on-disk format the strings in ecl_kw instances of type ECL_STRING and ECL_CHAR must be space padded to the correct length.
We will now create smspec nodes with WGNAME == DUMMY_WELL, however these smspec nodes will be marked with the valid flag set to false, and they will be ignored in the index.
The smspec_node_equal( ) function is implemented on top of the smspec_node_cmp( ) function. One important change from this is that the internal stoarge index - param_index - is no longer taken into account when comparing smspec nodes.
|
Fair enough, thanks for addressing! Please merge at will. |
|
Thar she blows! |
Task
This was mostly a wild goose chase trying to find a problem with the summary files generated by OPM. The third commit turned out to be the crucial one. The sorting is also valuable - although not as essential.
Approach
ECL_STRINGis space padded to correct length.Pre un-WIP checklist