Skip to content

(towards #2971) move parallel region trans#2972

Merged
LonelyCat124 merged 14 commits into
masterfrom
2971_move_trans_1
Jun 30, 2025
Merged

(towards #2971) move parallel region trans#2972
LonelyCat124 merged 14 commits into
masterfrom
2971_move_trans_1

Conversation

@victoria-atkinson

Copy link
Copy Markdown
Collaborator

No description provided.

@victoria-atkinson victoria-atkinson self-assigned this May 2, 2025
@codecov

codecov Bot commented May 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (86794cd) to head (a528fd5).
Report is 15 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2972   +/-   ##
=======================================
  Coverage   99.91%   99.92%           
=======================================
  Files         364      365    +1     
  Lines       51781    51785    +4     
=======================================
+ Hits        51739    51744    +5     
+ Misses         42       41    -1     

☔ 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.

@victoria-atkinson

Copy link
Copy Markdown
Collaborator Author

@arporter @sergisiso this is ready for review

@LonelyCat124 LonelyCat124 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @victoria-atkinson - looks good and tests all pass, I've got a couple of minor changes regarding documentation and "pythonicness" that would be good to fit in while we're updating this code.

In the mean time I'll set the integration tests as the remaining changes won't affect them.

Comment thread src/psyclone/tests/psyir/transformations/parallel_region_trans_test.py Outdated
Comment thread src/psyclone/psyir/transformations/parallel_region_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/parallel_region_trans.py Outdated
Comment thread src/psyclone/psyir/transformations/parallel_region_trans.py Outdated
@LonelyCat124

Copy link
Copy Markdown
Collaborator

Hi @victoria-atkinson - one tiny documentation thing to fix then I'll build all the documentation and should be ready to merge.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Going to resubmit integration test to double check.

@LonelyCat124 LonelyCat124 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi @vkatkinson - almost good now - I missed a couple of things in my last review.

We want most things to importable from the psyclone.psyir.transformations level or equivalent, so there needs to be an import of the Transformation in the __init__.py in that directory - for now that will mean that its importable from psyclone.psyir.transformations and psyclone.transformations, but I'm ok with this change and don't want a deprecation warning yet - we can add that later once all transformations are removed from transformations.py.
Alongside that change, can you try changing the import in transformations.py to just use the from psyclone.psyir.transformations import ParallelRegionTrans. If it breaks everything due to a cyclical import then leave it as it is, but I'm hoping it should work.

Also can you also move the location of the Sphinx AutoAPI declaration for this transformation from transformations.py to __init__.py (see line 3080 of transformations.py).

@vkatkinson

Copy link
Copy Markdown

@LonelyCat124 thanks Aidan. Think I've covered all your suggested changes.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Happy with this now, will merge.

@LonelyCat124 LonelyCat124 merged commit 7fcfa1d into master Jun 30, 2025
11 checks passed
@LonelyCat124 LonelyCat124 deleted the 2971_move_trans_1 branch June 30, 2025 15:50
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.

3 participants