-
Notifications
You must be signed in to change notification settings - Fork 13
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
IR: Move expr_visitors
to loki.ir
#372
Conversation
ec49adc
to
c56773d
Compare
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/372/index.html |
c56773d
to
cf9f4b3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
==========================================
- Coverage 95.48% 95.48% -0.01%
==========================================
Files 185 185
Lines 38651 38646 -5
==========================================
- Hits 36907 36901 -6
- Misses 1744 1745 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
cf9f4b3
to
a1d3bdf
Compare
a1d3bdf
to
189ce63
Compare
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.
Thank you very much for cleaning this up and the tedious work of updating all the imports accordingly. This is a fundamentally sensible thing to do and has been recognized also by others.
Changes look good to me!
Just testing for now...Note: This sits on top of #366, because it moves the new tests alongside it.
Another refactoring change that moves the definition of various IR visitors (
expr_visitors.py
) to theloki.ir
package. This means that now, at least in principle, we only have a one-way dependency betweenloki.ir
=>loki.expression
- and where this is broken, it is marked as such via local imports with pylint markers.Integration note: I'll leave this as draft until #366 is merged, so that the diff is not too overwhelming.