Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method getActiveIndexList to class ActiveList #2323

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

oddvarlia
Copy link
Contributor

@oddvarlia oddvarlia commented Nov 8, 2021

Add to python API the function for ActiveList that returns a list of indices of which parameters of a node is active. Add test for this function.

Purpose: Be able to write more precise tests for localisation using ActiveList

@ertomatic
Copy link
Collaborator

Can one of the admins verify this patch?

@sondreso
Copy link
Collaborator

sondreso commented Nov 9, 2021

Jenkins add to whitelist

res/enkf/active_list.py Outdated Show resolved Hide resolved
@oddvarlia oddvarlia force-pushed the active_list_values branch 4 times, most recently from 94e2292 to 0cf0db9 Compare November 15, 2021 07:21
@oddvarlia oddvarlia force-pushed the active_list_values branch 5 times, most recently from ebd244b to 83292ec Compare November 23, 2021 10:52
@oddvarlia oddvarlia changed the title Add test for ActiveList method getActiveIndexList Add method getActiveIndexList to class ActiveList Nov 23, 2021
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think the changes are fine, but think we should test this a bit more rigourously.

tests/libres_tests/res/enkf/test_active_list.py Outdated Show resolved Hide resolved
res/enkf/active_list.py Show resolved Hide resolved
@pinkwah
Copy link
Contributor

pinkwah commented Nov 29, 2021

Two notes:

  1. PEP8 function naming: get_active_index_list (yes, even though the rest of the file is camelCase)
  2. Use pytest tests for all new tests. I'd just put a def test_get_active_list() at the bottom of the test file.

@oddvarlia oddvarlia force-pushed the active_list_values branch 3 times, most recently from 483c8ce to ab0885b Compare November 30, 2021 13:26
@oddvarlia oddvarlia force-pushed the active_list_values branch 3 times, most recently from 3f1fecc to 6b601b6 Compare December 2, 2021 20:03
res/enkf/active_list.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2021

Codecov Report

Merging #2323 (2ad46eb) into main (be17fbe) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2323      +/-   ##
==========================================
+ Coverage   64.79%   64.80%   +0.01%     
==========================================
  Files         647      647              
  Lines       53995    54006      +11     
  Branches     4491     4491              
==========================================
+ Hits        34986    34999      +13     
  Misses      17573    17573              
+ Partials     1436     1434       -2     
Impacted Files Coverage Δ
res/enkf/active_list.py 97.67% <100.00%> (+0.79%) ⬆️
libres/lib/res_util/block_fs.cpp 53.51% <0.00%> (+0.14%) ⬆️
libres/lib/enkf/block_fs_driver.cpp 81.92% <0.00%> (+0.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be17fbe...2ad46eb. Read the comment docs.

@oddvarlia oddvarlia force-pushed the active_list_values branch 2 times, most recently from 8486f2c to f76d448 Compare December 7, 2021 07:49
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it also possible to add a test for ÌNACTIVE`? or is that state hard to get to?

tests/libres_tests/res/enkf/test_active_list.py Outdated Show resolved Hide resolved
tests/libres_tests/res/enkf/test_active_list.py Outdated Show resolved Hide resolved
tests/libres_tests/res/enkf/test_active_list.py Outdated Show resolved Hide resolved
@oddvarlia oddvarlia force-pushed the active_list_values branch 2 times, most recently from 1c62779 to f0d457e Compare January 3, 2022 12:16
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Purpose: Make it possible to check and use active parameters and
active observations. Enables better testing of the localisation scripts.
Add test for ActiveList.get_active_index_list().
@oyvindeide oyvindeide merged commit 8280eb1 into equinor:main Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Extend the class ActiveList with method to retrieve list of active observations or parameters
7 participants