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

(Towards #2668) Implementation of keyword arguments to transformations instead of options dict. #2877

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

LonelyCat124
Copy link
Collaborator

@LonelyCat124 LonelyCat124 commented Jan 29, 2025

This is still an early draft - documentation not yet handled.
This doesn't replace options at current, merely provides an alternative, more pythonic approach. Removing options would be a future issue for a large release of PSyclone, unlikely to happen in the immediate future.

…o replace options. Docs not yet done, need to add it to a subclass to do real testing
@LonelyCat124 LonelyCat124 changed the title (Towards #2668) Initial buildable implementation of parallel loop trans with kwargs t… (Towards #2668) Implementation of keyword arguments to transformations instead of options dict. Jan 29, 2025
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 90.32258% with 21 lines in your changes missing coverage. Please review.

Project coverage is 99.85%. Comparing base (10a3a6e) to head (acef4c4).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/psyclone/utils.py 77.77% 18 Missing ⚠️
src/psyclone/tests/psyGen_test.py 90.00% 2 Missing ⚠️
src/psyclone/psyGen.py 98.36% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2877      +/-   ##
==========================================
- Coverage   99.89%   99.85%   -0.05%     
==========================================
  Files         359      359              
  Lines       50995    51178     +183     
==========================================
+ Hits        50940    51102     +162     
- Misses         55       76      +21     

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

@LonelyCat124
Copy link
Collaborator Author

LonelyCat124 commented Jan 30, 2025

@arporter @sergisiso I added a basic idea for the decorator to the utils.

It currently doesn't work correctly in a couple of ways:

  1. It doesn't update validate from apply (not worth doign yet)
  2. It doesn't have type checking in docstrings correct yet. E.g. if we have something like :param bool myname: it can't yet determine that the name of the variable is myname with type bool at this point (or even worse if its like bool | int).

Point 1 is fine to fix once I'm happy with the apply change.
Point 2 is a little more tricky - I think I need to change my regex from what it is now to one that checks for 2 groups, one which can include whitespace (only around a pipe?) and alphanumeric_ characters and then check if there is 1 group or 2 when pulling name/type info.

I've not yet tested what sphinx gives me for it - python doc is correctly updated, I'll try to build the docs and check ParallelLoopTrans docs.

Update: Sphinx does appear to give me something I expect - there are warnings on github that I can't reproduce locally, but it seems to just be missing blank lines I think? So potentially an easy fix. The picture below has inherited the verbose parameter from its parent class.
There are also something weird in this dosctring for verbose's type, that will need fixing - this is just from the options["verbose"] and is a followon line so that a mistake in my code is all - shouldn't be a big fix.
The missing docs are arguments that belong to ParallelLoopTrans that are just not yet documented in its own docstring,
image

@LonelyCat124
Copy link
Collaborator Author

Docs still fail here, but I can't duplicate it locally so I can't diagnose it - for me the docs build and look ok. There's some warning to do with line indentation/breaks but I can't work out what it means.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant