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.

Sign up for GitHub

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

Jump to bottom

Improve graph parameter support #2527

Merged
merged 4 commits into from
Jan 14, 2018
Merged

Improve graph parameter support #2527

merged 4 commits into from
Jan 14, 2018

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Jan 5, 2018 •
edited
Loading

Address case 2, 3 and 4 of #2525:

  • Allow out of range parameter on graph RHS.
  • Allow positive offset for parameter index on graph.
  • Allow negative integer parameters.

@matthewrmshin matthewrmshin added the bug Something is wrong :( label Jan 5, 2018
@matthewrmshin matthewrmshin added this to the next release milestone Jan 5, 2018
@matthewrmshin matthewrmshin self-assigned this Jan 5, 2018
@hjoliver hjoliver mentioned this pull request Jan 7, 2018
Closed
4 tasks
@matthewrmshin
Fix out of range parameter on graph RHS
9606ae8
@matthewrmshin
More parameters fixes
4e58ed0 
Negative integer parameters.
Positive and negative offsets in graph.
@matthewrmshin matthewrmshin force-pushed the fix-param-out-of-range-rhs branch from c9b2e4f to 4e58ed0 Compare January 8, 2018 15:30
@matthewrmshin matthewrmshin changed the title Fix out of range parameter on graph RHS Improve graph parameter support Jan 8, 2018
hjoliver
hjoliver approved these changes Jan 11, 2018
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Good, tests as working. 3. and 4. need a quick mention in the parameters chapter of the user guide though (2. is just a bug).

@matthewrmshin
Doc -ve integer parameter and graph +ve offset
b49ae52 
Also improve default template for parameter lists with -ve integers.
@matthewrmshin
Copy link
Contributor Author

On adding some docs for negative integer parameters, I have realised that the default template (for task name suffix) should also be updated as well. I have now gone for the simple rule where:

  • As before - for parameters with only positive integer values.
  • Add a sign to all values if any parameter value is negative. E.g.: for i = -1..1, graph = foo<i> will generate task names foo_i-1, foo_i+0 and foo_i+1 by default.

oliver-sanders
oliver-sanders approved these changes Jan 12, 2018
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I'm unable to break it! Leaving to @hjoliver to merge since new commits have been added.

@hjoliver hjoliver merged commit f963692 into cylc:master Jan 14, 2018
@hjoliver
Copy link
Member

(Several Travis CI failures post-merge).

@matthewrmshin matthewrmshin deleted the fix-param-out-of-range-rhs branch January 15, 2018 08:26
@matthewrmshin
Copy link
Contributor Author

(Travis CI failures unrelated to this change. Investigating.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hjoliver hjoliver hjoliver approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@matthewrmshin matthewrmshin

Labels
bug Something is wrong :(
Projects
None yet
Milestone
cylc-7.6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@matthewrmshin @hjoliver @oliver-sanders