-
Notifications
You must be signed in to change notification settings - Fork 33
(Closes #3087) Handle expressions in the kind attribute of a declaration #3110
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3110 +/- ##
==========================================
- Coverage 99.90% 99.90% -0.01%
==========================================
Files 376 376
Lines 52816 52887 +71
==========================================
+ Hits 52766 52835 +69
- Misses 50 52 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sergisiso This is ready for a first look. There is a "logging" message commented out in the old code at |
|
I fixed up the functionality now I think, I need to have a look through the docstrings of the primary changes to check I didn't break anything. |
|
I'm not sure if this solves something to do with #2659? |
…ne into 3087_binary_expression_kinds
|
@sergisiso I think this is ready for a first look now, am hoping I didn't miss anything w.r.t docstrings. We can try to update some more to use type hints if you'd like, but since this is already quite large I didn't do anything with that yet (especially as I think I may hit more circular dependency issues) |
|
Running integration tests to see if #2659 is still an issue there... |
sergisiso
left a comment
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.
@LonelyCat124 There is nothing new other than a couple of pylint suggestions, the rest are just the continuation of previous conversations that I brought down here as it was hard to keep track of them having to expand the history.
After this, can you bring it to master and launch the integration tests, as I expect these would be the last comments.
src/psyclone/domain/common/transformations/kernel_module_inline_trans.py
Outdated
Show resolved
Hide resolved
src/psyclone/psyir/transformations/mark_routine_for_gpu_mixin.py
Outdated
Show resolved
Hide resolved
src/psyclone/tests/domain/common/transformations/kernel_module_inline_trans_test.py
Outdated
Show resolved
Hide resolved
|
@sergisiso updated comments for review, going to set integration going and see how it does. Edit: Hmm I broke lots of things with coverage and docs. I'll keep looking. |
|
The integration tests were green though, I'll see what i've broken next. Edit: doctest was link checking failing, can see if thats fixed when i recommit. |
|
I removed some code that I believe is dead (at least there was no way I could reach it in tests), I think we should rerun integration again to be sure before merging. |
|
@sergisiso I think this is ready for another look - the only missing coverage is the LFRic code that is untested on master I think. |
|
@LonelyCat124 Unfortunately the very last merged PR also touched declarations and has conflicts, can resolve these and launch the integration once again please. |
|
@sergisiso Ready for review now - I'll set integration running. |
sergisiso
left a comment
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.
@LonelyCat124 This is ready to merge, there is a new indirect coverage miss (apart from the lfric one). I was going to propose a solution but its not very nice and I think we should fix the underlaying problem that I explained in #3169, so I defer the fix to that separate issue.
First implementation to handle kinds like
kind=2*wp. Not sure if I've missed anything as I'm not super knowledgable about what kinds can be - maybe there's some missing error checking or similar.Still have a few failing tests to address.