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

Liu 303 - Correcting Scheduler Algorithms #450

Merged
merged 5 commits into from
Sep 27, 2022
Merged

Liu 303 - Correcting Scheduler Algorithms #450

merged 5 commits into from
Sep 27, 2022

Conversation

pritchardn
Copy link
Contributor

@pritchardn pritchardn commented Sep 20, 2022

This sibing PR of ICRAR/daliuge/pull/202, removes the simple and none translation options and adds No. of Nodes to the remaining options.
The default parameters provided should successfully translate a graph for all options.

@pritchardn pritchardn marked this pull request as ready for review September 20, 2022 04:21
Copy link
Contributor

@james-strauss-uwa james-strauss-uwa left a comment

Choose a reason for hiding this comment

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

Looks good.

src/Config.ts Outdated
@@ -36,8 +36,7 @@ export class Config {
"metis",
"mysarkar",
"min_num_parts",
"pso",
"simple"
"pso"
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems weird that we just remove one element here, but I can see that removing the first element will require us to re-index all the genPGT() calls in the html. So it makes sense to leave element 0 in place.

Maybe just add a comment that element 0 ("none") is not used.

OR maybe we should just remove the whole "translationAlgorithms" variable, and move the strings ("metis", "mysarkar" etc) directly into the genPGT() calls, changing the first argument to genPGT() from algorithmIndex:number to algorithmName:string?

I don't think we use the translationAlgorithms variable anywhere else, so maybe we can just remove one layer of unnecessary indirection.

Up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea of changing the variable from an index to the string directly so I'll go ahead and do that.
There is some method to only removing a single element - in the API 'none' is a valid 'algorithm' choice, it just doesn't work at the moment. Simple on the other hand, doesn't exist at all.

@pritchardn pritchardn merged commit 7e7913e into master Sep 27, 2022
@pritchardn pritchardn deleted the LIU-303 branch September 27, 2022 05:18
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.

None yet

2 participants