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

int parameters can now be formatted by %d #2431

Merged

Conversation

matthewrmshin
Copy link
Contributor

@matthewrmshin matthewrmshin commented Sep 19, 2017

Allow and test usage of a %d format in a parameter template (for task suffixes).

@matthewrmshin matthewrmshin added this to the next release milestone Sep 19, 2017
@matthewrmshin matthewrmshin self-assigned this Sep 19, 2017
@matthewrmshin matthewrmshin force-pushed the allow-percent-d-tmpl-for-int-params branch from 4a0e6f8 to 0db4996 Compare September 20, 2017 07:52
@matthewrmshin matthewrmshin requested review from hjoliver, oliver-sanders and dvalters and removed request for hjoliver September 20, 2017 08:29
@matthewrmshin
Copy link
Contributor Author

Reassign @dvalters as reviewer while @hjoliver is offline.

@@ -296,7 +295,8 @@ def expand(self, line):
except ValueError:
nval = val
else:
nval = val.zfill(len(self.param_cfg[pname][0]))
nval = val.zfill(
len(str(self.param_cfg[pname][0])))
Copy link
Member

Choose a reason for hiding this comment

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

Why has this case been handled differently to the previous example https://github.com/cylc/cylc/pull/2431/files#diff-4b401b4bbdbea5e41b7f93c7b9ef92d8R157?

ivals = [str(i) for i in range(2)]
jvals = [str(j) for j in range(3)]
kvals = [str(k) for k in range(2)]
ivals = [i for i in range(2)]
Copy link
Member

Choose a reason for hiding this comment

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

list(range(2)) (actually just range(2) works for python 2.x)?

@matthewrmshin
Copy link
Contributor Author

All comments addressed.

It is now OK to use `%d` template for integer-only parameters.
[[parameters]]
p = 3..14
[[parameter templates]]
p = %%p%(p)03d # suffixes = %p003, %p004, ..., %p013, %p014
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be clearer without the escaped %:

p = p%(p)03d  # suffixes = p003, p004, ..., p013, p014

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case, I'll add another example. (I think it will still be useful to have an example on how to have a % in your template.)

@@ -4658,11 +4664,11 @@ \subsubsection{Selecting Partial Parameter Ranges}
\begin{lstlisting}
[cylc]
[[parameters]]
run = 1..10 # 01, 02, ..., 10
runx = 1..03 # 01, 02, 03 (note `03' to get correct padding)
run = 1..10 # 1, 2, ..., 10
Copy link
Member

Choose a reason for hiding this comment

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

This still results in 01, 02, ... 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The numbers are now stored internally as int 1, 2, ..., 10, but will then have a default template like _run%(run)02d, so the suffixes will continue to be _run01, _run02, ..., _run10.

[[parameter templates]]
run = _R%(run)s
runx = _R%(runx)s
run = _R%(run)02d
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps move the comments here i.e:

run = _R%(run)02d  # _R01, _R02, ..., _R10
runx = _R%(runx)02d  # _R01, _R02, _R03

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hear.

@matthewrmshin
Copy link
Contributor Author

matthewrmshin commented Sep 25, 2017

Updated as the original version of this comment was factually incorrect.

As an aside, it looks like we have a similar issue with $CYLC_TASK_PARAM_<x>. (This highlights the gap in our tests and docs).

Before #2360, the value was zero-padded for integer parameters with width fixed to the maximum as-defined string width. After #2360, the value is still zero-padded, but with width fixed to the width of the maximum integer value.

Zero padded decimal is:

  • Good for use in fixed width string values such as file names.
  • Not-so-good if a normal decimal number is required. (E.g. Bash and utilities such as printf interprets numbers with padded zeroes as an octal by default.)

The current change means that the value of $CYLC_TASK_PARAM_<x> will just be the decimal value with no padding. Objection?

Thinking aloud, perhaps it is worth having a bit more flexibility for the value (and even the naming of) the environment variable? The [[parameter templates]] section is a bit misleading, when its settings are really only used as task name suffixes. We can perhaps introduce a new setting to define custom environment variables based on parameters, as well as format strings for their values?

@matthewrmshin
Copy link
Contributor Author

Note: #2435 is somewhat related to this.

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.

Looks good. I like your idea for custom parameter environment variables - can you post an Issue for that?

@hjoliver hjoliver merged commit 9631207 into cylc:master Sep 28, 2017
@matthewrmshin matthewrmshin deleted the allow-percent-d-tmpl-for-int-params branch September 28, 2017 18:15
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.

4 participants