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

2451 DynInvokeSchedule split & rename #2452

Merged
merged 24 commits into from
May 16, 2024

Conversation

oakleybrunt
Copy link
Collaborator

@oakleybrunt oakleybrunt commented Jan 3, 2024

closes #2451

@oakleybrunt oakleybrunt self-assigned this Jan 3, 2024
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (77510b9) to head (4164336).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2452   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files         356      357    +1     
  Lines       47922    47929    +7     
=======================================
+ Hits        47853    47860    +7     
  Misses         69       69           

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

@oakleybrunt oakleybrunt changed the title 2451 split rename dyn invoke schedule 2451 DynInvokeSchedule split & rename Jan 8, 2024
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.86%. Comparing base (8ccdebe) to head (bf9d495).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2452   +/-   ##
=======================================
  Coverage   99.86%   99.86%           
=======================================
  Files         353      354    +1     
  Lines       48286    48293    +7     
=======================================
+ Hits        48223    48230    +7     
  Misses         63       63           

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

@TeranIvy TeranIvy self-requested a review May 9, 2024 10:19
@oakleybrunt
Copy link
Collaborator Author

@TeranIvy - tests now passing, ready for review :)

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Good job. I only managed to find one outstanding mention of DynInvokeSchedule:

./src/psyclone/tests/psyir/tools/call_tree_utils_test.py:    # This will return three schedule - the DynInvokeSchedule, and two

Ref. guide still builds fine, as does the UG. I'll fire off the integration tests but as these don't currently work for LFRic, I'll need independent verification from you that you tested this branch with LFRic :-)

src/psyclone/domain/lfric/lfric_invoke_schedule.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_invoke_schedule.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_invoke_schedule.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All requested changes have been made (thanks) and integration tests were fine.
@oakleybrunt please could you confirm that you've tested LFRic with this branch?
Note: reference guide still builds fine.

@arporter
Copy link
Member

arporter commented May 15, 2024

@oakleybrunt, re. building with LFRic, Sergi is hopeful that he will have the integration tests fixed today so we can just wait for them to come back :-)

@oakleybrunt
Copy link
Collaborator Author

@oakleybrunt, re. building with LFRic, Sergi is hopeful that he will have the integration tests fixed today so we can just wait for them to come back :-)

Okay, I'll get Iva to run me through the process when she's back from leave for future reference :)

@arporter
Copy link
Member

I've just triggered the integration tests. If they are OK then I'll proceed to merge.

@oakleybrunt
Copy link
Collaborator Author

I've just triggered the integration tests. If they are OK then I'll proceed to merge.

Thanks Andy, sorry for not getting to the LFRic testing myself.

@arporter
Copy link
Member

Everything was green. will proceed to merge.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

All good now.
Just waiting on final CI.

@arporter arporter merged commit f3146e1 into master May 16, 2024
11 checks passed
@arporter arporter deleted the 2451_split_rename_DynInvokeSchedule branch May 16, 2024 12:38
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.

Split and rename DynInvokeSchedule from dynamo0p3.py
3 participants