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

Assumptions for work depth analysis #234

Merged
merged 11 commits into from
Oct 2, 2023
Merged

Assumptions for work depth analysis #234

merged 11 commits into from
Oct 2, 2023

Conversation

hodelcl
Copy link
Contributor

@hodelcl hodelcl commented Sep 4, 2023

This PR enables the user to enter assumptions for the symbols before calling the work depth analysis.

It uses a VS code InputBox to handle the input. Validity of the input is checked by a regex. If the check passes, the assumptions are forwarded to the work depth analysis.

The corresponding PR to the dace repo can be found here.

Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

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

Very nice, thank you :-) Small changes only

this.genericErrorHandler(error.message, error.details);
reject(error.message);

vscode.window.showInputBox({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is quite some code repetition, is it possible to factor this out into a separate small function? (Showing the prompt, checking for the format, showing error message if violated, etc.)

@@ -53,7 +53,7 @@

sys.path.append(path.abspath(path.dirname(__file__)))

from dace_vscode import arith_ops, depth, transformations
from dace_vscode import depth, transformations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't the dace_vscode.arith_ops module be removed entirely now?


from dace_vscode.utils import (
load_sdfg_from_json,
get_exception_message,
)

def get_work(sdfg_json, assumptions):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This module should now probably be renamed to work_depth or something :-)

@hodelcl
Copy link
Contributor Author

hodelcl commented Sep 28, 2023

I integrated all your comments:

  • put the InputBox for the assumptions and the input checking in separate functions,
  • deleted the dace_vscode.arith_ops module,
  • renamed the depth module to work_depth.

@hodelcl hodelcl requested a review from phschaad September 28, 2023 07:30
Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@phschaad phschaad merged commit 786ded7 into spcl:master Oct 2, 2023
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.

2 participants