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

Scan keywords with roffio when importing GridProperties #954

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

mferrera
Copy link
Collaborator

@mferrera mferrera commented Nov 1, 2023

Resolves #939

This test failure is caused by

/* add a third item if parameter name */
if (strncmp(cname[j], "name", 4) == 0 &&
strncmp(pname[j], "NAxxx", 2) != 0) {
strcat(tagletters, "!");
strcat(tagletters, pname[j]);
}

This will ignore any properties starting with "NA", which is probably not what we want. This PR changes to scan keywords with roffio instead, at a minor performance hit. We could probably seek to replace the C scan with roffio completely, but the GridProperties.scan_keywords() method has a return format that roffio cannot support.

Benchmarks:

  • xtgeo-testdata/1/geogrid.roff
    • roffio: 0.013
    • cxtgeo: 0.003
  • xtgeo-testdata/2/geogrid.roff
    • roffio: 0.013
    • cxtgeo: 0.003
  • xtgeo-testdata/3/valysar.roff
    • roffio: 0.013
    • cxtgeo: 0.003

@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2023

Codecov Report

Merging #954 (378f593) into main (69d8caa) will decrease coverage by 0.05%.
The diff coverage is 82.22%.

@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   80.57%   80.53%   -0.05%     
==========================================
  Files          91       91              
  Lines       13488    13501      +13     
  Branches     2224     2229       +5     
==========================================
+ Hits        10868    10873       +5     
- Misses       1899     1904       +5     
- Partials      721      724       +3     
Files Coverage Δ
src/xtgeo/grid3d/grid_properties.py 86.22% <100.00%> (+0.31%) ⬆️
src/xtgeo/grid3d/_gridprops_import_roff.py 90.32% <85.71%> (-9.68%) ⬇️
src/xtgeo/grid3d/_grid3d_utils.py 85.08% <66.66%> (-5.83%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mferrera mferrera force-pushed the fix-gprops-test branch 3 times, most recently from 2af8fde to 378f593 Compare November 1, 2023 10:16
@mferrera mferrera marked this pull request as ready for review November 1, 2023 10:28
@mferrera mferrera requested a review from janbjorge November 1, 2023 10:28
@mferrera mferrera self-assigned this Nov 1, 2023
@mferrera mferrera requested a review from tnatt November 3, 2023 06:40
@jcrivenaes
Copy link
Collaborator

I would the think the issue in #939 is the "<" letters, which I am pretty sure that Eclipse and friends will not allow in keywords.

We could probably seek to replace the C scan with roffio completely, but the GridProperties.scan_keywords() method has a return format that roffio cannot support.

If it is the byte position reports that are not supported, I think that is something which can be safely changed, and end users never will have use of that. Would it then be possible to scan with roffio completely?

@mferrera
Copy link
Collaborator Author

mferrera commented Nov 6, 2023

If it is the byte position reports that are not supported, I think that is something which can be safely changed, and end users never will have use of that. Would it then be possible to scan with roffio completely?

Yep!

It is specifically the strncmp(pname[j], "NAxxx", 2) != 0 condition that causes this fail; if the first two characters of pname[j] are "NA" this strncmp will return 0, so this condition won't be true and the parameter won't be added to the scanned list.

Copy link
Collaborator

@jcrivenaes jcrivenaes left a comment

Choose a reason for hiding this comment

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

LGTM

When importing a roff GridProperties file the names of the properties
were scanned with C code which fails to collect keynames that begin
with "NA". This change instead collects the valid keynames with
roffio. Benchmark testing with roff files of different sizes showed
that this incurs a consistent performance drop of 0.01 seconds.
Touched on the `scan_keywords` method while working looking through
`_grid3d_utils.scan_keywords` invocations.
@mferrera mferrera merged commit 3a3507c into equinor:main Nov 6, 2023
@mferrera mferrera deleted the fix-gprops-test branch November 6, 2023 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate and fix failing GridProperties test
4 participants