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

Expression: Expression cloning and mapper tests #419

Merged
merged 12 commits into from
Nov 7, 2024

Conversation

mlange05
Copy link
Collaborator

@mlange05 mlange05 commented Oct 30, 2024

Note: Only for testing so far ...

The primary intention behind this PR is to increase test coverage of the expression mappers and rigorously duplicate symbols when expression mappers (LokiIdentityMapper and SubstituteExpressionMapper). It also fixes a bunch of other issues along the way that this threw up and now fixes #379 .

In a little more detail:

  • Add dedicated tests for expression walker (via ExpressionRetriever) and the mappers LokitIdentityMapper and SubstituteExrpressionMapper
  • Lint-clean the expression tests directory
  • Remove the now redundant invalidate_source argument from Loki mappers (we do not store source on expressions anymore)
    • Fix the resulting issue in recursive_expression_map_update by checking for ancestry of SubstituteExpressionMapper before handing the map to constructors
  • Duplicate symbols in LokiIdentityMapper and SubstotuteExpressionMapper
    • Add a local _rebuild() method the base class that check for scope-carrying and scope-free symbols
    • Add a dedicated symbols test for correct re-instatiation of all our symbols types and fix the ones that do not behave
    • Always update the symbol after enrichment, to ensure that DeferredTypeSymbols are replaced with ProcedureTypeSymbols in CallStatement.name after the type updates.

Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/419/index.html

@mlange05 mlange05 force-pushed the naml-expression-mapper-tests branch from 8298e0c to 97cb7a9 Compare October 30, 2024 10:48
@mlange05 mlange05 changed the title Expression: Expression cloning and built-in recursion Expression: Expression cloning and mapper tests Oct 30, 2024
@mlange05 mlange05 force-pushed the naml-expression-mapper-tests branch from 7f0c0ed to 47a7b9b Compare October 31, 2024 15:38
@mlange05 mlange05 force-pushed the naml-expression-mapper-tests branch 2 times, most recently from 2f57346 to 76f1360 Compare November 1, 2024 06:08
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 98.62069% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.11%. Comparing base (899f5ac) to head (193742c).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
loki/expression/mappers.py 89.47% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
+ Coverage   93.04%   93.11%   +0.06%     
==========================================
  Files         198      200       +2     
  Lines       39320    39550     +230     
==========================================
+ Hits        36587    36827     +240     
+ Misses       2733     2723      -10     
Flag Coverage Δ
lint_rules 96.39% <ø> (ø)
loki 93.06% <98.62%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mlange05 mlange05 force-pushed the naml-expression-mapper-tests branch from 76f1360 to 8977596 Compare November 1, 2024 07:53
@mlange05 mlange05 force-pushed the naml-expression-mapper-tests branch from 8977596 to 29c8a15 Compare November 1, 2024 08:16
@mlange05 mlange05 requested a review from reuterbal November 1, 2024 08:16
@mlange05 mlange05 marked this pull request as ready for review November 1, 2024 08:16
@mlange05
Copy link
Collaborator Author

mlange05 commented Nov 1, 2024

FYI: Checking with ec-physics regression locall, there is no noticeable performance impact on loki processing speed from this.

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

This is fantastic, many thanks. I also appreciate the number of clean-ups along the way, like the stray source handling for expression nodes.

Only comment is a suggestion for a stricter test, but otherwise nothing to add.

@Andrew-Beggs-ECMWF would you mind confirming if this resolves your issue?

loki/expression/tests/test_symbols.py Outdated Show resolved Hide resolved
@reuterbal reuterbal added the ready to merge This PR has been approved and is ready to be merged label Nov 7, 2024
@reuterbal reuterbal merged commit f731b0b into main Nov 7, 2024
13 checks passed
@reuterbal reuterbal deleted the naml-expression-mapper-tests branch November 7, 2024 14:06
@Andrew-Beggs-ECMWF
Copy link
Contributor

This is fantastic, many thanks. I also appreciate the number of clean-ups along the way, like the stray source handling for expression nodes.

Only comment is a suggestion for a stricter test, but otherwise nothing to add.

@Andrew-Beggs-ECMWF would you mind confirming if this resolves your issue?

I'd like to reiterate Balthasar's comments, I'm really please to see this patch. Thank you for doing it! 😁

I can confirm all my tests now pass!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR has been approved and is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Literals not being rebuilt causing shallow copy issues
3 participants