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

Update string format representation used in instrument file output #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

g5t
Copy link
Contributor

@g5t g5t commented Jun 8, 2023

Background

When a parameter needs to be written into a McStas/McXtrace .instr file McStasScript makes use of printf-style format specifiers, notably %d, %s, and %G.
Importantly, the last is used for floating point values which seems reasonable since, quoting the Wikipedia entry for %g/%G

double in either normal or exponential notation, whichever is more appropriate for its magnitude. g uses lower-case letters, G uses upper-case letters. This type differs slightly from fixed-point notation in that insignificant zeroes to the right of the decimal point are not included. Also, the decimal point is not included on whole numbers.

Problem

Unfortunately in some cases this style of string formatting can lead to truncation for floating point values, e.g.,

>>> x = 1.234567
>>> '%G' % x
'1.23457'

where the last digit has been truncated and the resulting string representation is rounded up.
This behaviour led to the discovery of a bug in Elliptic_guide_gravity.comp where the lengths of a number of guide segments were written by McStasScript into a DECLARE array and their total length was written by McStasScript as a component parameter. Truncation of both the individual element lengths and their total length produced a C instrument file where the sum of the segment lengths and the input total length were no longer in agreement.

Solution

This PR corrects this problem by moving to f-string formatting and dropping the format specifier for str and float values, since Python does a good job of accurately representing values via str() already.
For example, the value above is no longer truncated

>>> x = 1.234567
>>> f'{x}'
'1.234567'

and this works for numbers with large-magnitude exponents as well

>>> x = 1.23456789e29
>>> f'{x}'
'1.23456789e+29'

Side effects

A benefit of f-strings in Python is that the resulting code is easier to read, with the position of values directly related to their position in the resulting string.
For example,

fo.write(f"{self.type} {self.name} = {self.value}; {self.comment}")

or

fo.write(f"AT {tuple(self.AT_data[:3])}")

@g5t
Copy link
Contributor Author

g5t commented Jun 9, 2023

My latest commit is due to changes I made, visible in the 'side effects' section above, that caused test cases to fail.

  1. I did not expect self.comment to include a space before the one-line comment marker, e.g., `' //...'.
  2. Python's string representation of a tuple is a parenthesized list separated by commas and spaces, but the old behavior did not include spaces, i.e., old (0,0,0) new (0, 0, 0).

For the first case I changed the formatting lines to remove the space, ...{self.value};{self.comment}.
For the second I changed the test cases to add spaces since it makes no difference on the C side and I think the Python representation is easier to read. This can be changed back if the old behavior is important for some reason.

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