Skip to content

(Closes #3366) rm LFRicSymbolTable#3374

Merged
LonelyCat124 merged 16 commits intomasterfrom
3366_rm_lfricsymboltable
Apr 21, 2026
Merged

(Closes #3366) rm LFRicSymbolTable#3374
LonelyCat124 merged 16 commits intomasterfrom
3366_rm_lfricsymboltable

Conversation

@arporter
Copy link
Copy Markdown
Member

@arporter arporter commented Mar 13, 2026

This branch was taken from the one for #1734 and therefore needs #3358 to be merged first.

@arporter arporter marked this pull request as draft March 13, 2026 15:50
@arporter arporter self-assigned this Mar 13, 2026
@arporter arporter changed the title (Closes #3366) rm lfricsymboltable (Closes #3366) rm LFRicSymbolTable Mar 13, 2026
@arporter arporter added the Blocked An issue/PR that is blocked by one or more issues/PRs. label Mar 16, 2026
@arporter arporter added in progress and removed Blocked An issue/PR that is blocked by one or more issues/PRs. labels Mar 23, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (31a7191) to head (80cb9eb).
⚠️ Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3374      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files         390      389       -1     
  Lines       54563    54521      -42     
==========================================
- Hits        54544    54502      -42     
  Misses         19       19              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@arporter arporter marked this pull request as ready for review April 1, 2026 12:43
@arporter
Copy link
Copy Markdown
Member Author

arporter commented Apr 1, 2026

ITs are underway but this is ready for a first look. One for @sergisiso, @hiker or @LonelyCat124 .

Copy link
Copy Markdown
Collaborator

@LonelyCat124 LonelyCat124 left a comment

Choose a reason for hiding this comment

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

Hi @arporter - mostly looks good, I have a couple of questions about functionality because I'm not sure what happens if find_or_create finds a symbol already in the table with the wrong datatype (or if thats possible)? From what I can see I don't see any type checking in find_or_create.

I'm somewhat loathe to suggest it, but could we get more type hints in the functions you've modified if that feels reasonable? I'm not sure what our rules w.r.t adding those in is so feel free to ignore if you think its too much.

Comment thread src/psyclone/domain/lfric/lfric_builtins.py
Comment thread src/psyclone/transformations.py
@arporter
Copy link
Copy Markdown
Member Author

arporter commented Apr 8, 2026

Thanks Aidan. I've done the type hinting (which was harder than I expected because of having to import classes and then fix circular dependencies). The find_or_create limitation is the subject of #1057 so I've not done that.

Copy link
Copy Markdown
Collaborator

@LonelyCat124 LonelyCat124 left a comment

Choose a reason for hiding this comment

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

Looks good now, I've set integration going and assuming no issues arise I'll proceed to merge.

Comment thread src/psyclone/domain/lfric/lfric_builtins.py
@arporter
Copy link
Copy Markdown
Member Author

The LFRic IT with nvfortran failed with a signal 11 so I've launched it a second time.

@LonelyCat124 LonelyCat124 merged commit aefaba4 into master Apr 21, 2026
14 of 15 checks passed
@LonelyCat124 LonelyCat124 deleted the 3366_rm_lfricsymboltable branch April 21, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants