Add round trip test for analysis::load_parameters and analysis::save_… #2711
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2711 +/- ##
==========================================
+ Coverage 64.90% 65.32% +0.41%
==========================================
Files 651 651
Lines 53687 53655 -32
Branches 4733 4724 -9
==========================================
+ Hits 34846 35050 +204
+ Misses 17363 17071 -292
- Partials 1478 1534 +56
Continue to review full report at Codecov.
|
ManInFez
force-pushed
the
testing_save_parameters
branch
2 times, most recently
from
ec2356d
to
9861f06
Compare
|
test this please |
ManInFez
force-pushed
the
testing_save_parameters
branch
from
0c4f0e2
to
7bad165
Compare
|
Test ert please! |
|
I see you have two separate issues for save and load (#2581 and #2583). I see that this test covers load and thus implicitly covers save, but are their assertions that we want to invoke on save (#2581)? because we can do it in this issue. with another SENARIO(""){
WHEN("save"){
THEN(""){
save assersions...
}
}
WHEN("load"){
THEN(""){
load assersions...
}
}
} |
|
Test ert please! |
| matrix_type *load_parameters(enkf_fs_type *target_fs, | ||
| ensemble_config_type *ensemble_config, | ||
| const int_vector_type *iens_active_index, | ||
| int last_step, int active_ens_size, |
There was a problem hiding this comment.
The parameter active_ens_size is not required - that equals int_vector_size( iens_active_index );
There was a problem hiding this comment.
this does not work when only running a subset of realizations 🤷
There was a problem hiding this comment.
OK I agree;
ens_size = int_vector_size( iens_active_index ) - int_vector_count_equal( iens_active_index, int_vector_get_default( iens_active_index));
not sure if there are any meaningful assertions to be made after save_parameters which are not covered by load_parameters |
|
Test ert please! |
ManInFez
force-pushed
the
testing_save_parameters
branch
from
c916012
to
7bad165
Compare
| } | ||
|
|
||
| // Create matrix and save as as the parameter defined in the ministep | ||
| matrix_type *A = matrix_alloc(1, 10); |
There was a problem hiding this comment.
I think the literal 10 should be named symbol ens_size
| void save_parameters(enkf_fs_type *target_fs, | ||
| ensemble_config_type *ensemble_config, | ||
| const int_vector_type *iens_active_index, int last_step, | ||
| const local_ministep_type *ministep, matrix_type *A); |
There was a problem hiding this comment.
Could the input-matrix A be defined as const?
There was a problem hiding this comment.
since it is passed to the serialize_info_alloc wich is also used in load_parameters it can not be const
There was a problem hiding this comment.
Is this something we should consider redesigning as it is a bit hard to parse, for me at least.
I would expect a function called save_parameters to be able to take const matrix_type *A.
Not saying we should do it in this PR!
ManInFez
force-pushed
the
testing_save_parameters
branch
from
010dab6
to
ef76101
Compare
…parameters Improve arguments for load/save_parameters
ManInFez
force-pushed
the
testing_save_parameters
branch
from
c8ff915
to
c8a8788
Compare
…parameters
Issue
Resolves #2581
Resolves #2583
Approach
Performing a roundtrip test where a matrix is first written to a enkf_fs instance and the loaded back
Pre review checklist
Adding labels helps the maintainers when writing release notes, see sections and the
corresponding labels here: https://github.com/equinor/ert/blob/main/.github/release.yml