-
Notifications
You must be signed in to change notification settings - Fork 12
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
SCC test reshuffle #353
SCC test reshuffle #353
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/353/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #353 +/- ##
=======================================
Coverage 95.35% 95.35%
=======================================
Files 171 173 +2
Lines 36801 36835 +34
=======================================
+ Hits 35090 35124 +34
Misses 1711 1711
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's an excellent idea!
There's now a redundant fixture definition in each of the two files, otherwise good to go!
@pytest.fixture(scope='module', name='vertical') | ||
def fixture_vertical(): | ||
return Dimension(name='vertical', size='nz', index='jk', aliases=('nlev',)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixture seems unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot! I just pushed a fix.
@pytest.fixture(scope='module', name='horizontal_bounds_aliases') | ||
def fixture_horizontal_bounds_aliases(): | ||
return Dimension( | ||
name='horizontal_bounds_aliases', size='nlon', index='jl', | ||
bounds=('start', 'end'), aliases=('nproma',), | ||
bounds_aliases=('bnds%start', 'bnds%end') | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and this fixture seems unused here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this one is still used:
https://github.com/ecmwf-ifs/loki/blob/naml-scc-test-reshuffle/loki/transformations/single_column/tests/test_scc_vector.py#L131
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, flagged it on the wrong file 🤦 I think that fixture is unused in test_scc_hoist.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, gotcha now! Just "fixed the fix" and updated for one more round of CI goodness. 😎
9362874
to
99a6a30
Compare
Small, purely cosmetic housekeeping change that splits the dedicated "scc_hoist" and vectorisation tests into separate sub-modules and renames the main SCC test to
test_scc.py
for consistency. The diff looks big, but other than test names no functionality was changed.