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

STYLE: Fix filename and dirname creation floating point formatting #232

Merged

Conversation

jhlegarreta
Copy link
Contributor

Fix formatting of numbers that can be floats when creating filenames and dirnames: when transitioning to f-strings (commit ac6040f), it was assumed that sigma and nonrigid_grid_resolution would be integers, based on the existing formatting. However, these attributes can be set to non-integer values. The pre-f-string transition solution, e.g.:

"iteration_%05d_sigma_%05d" % (self.total_iterations, self.sigma)

trimmed the decimal part, and thus, for e.g. total_iterations 20 and sigma 7.5, it would produce:

'iteration_00020_sigma_00007'

With the introduction of f-strings, i.e.

f"iteration_{self.total_iterations:05d}_sigma_{self.sigma:05d}"

this would produce an error, since the sigma floating point value cannot be formatted as an integer:

{ValueError}ValueError("Unknown format code 'd' for object of type
'float'")

This patch set fixes the error by adopting the previous behavior: it trims the decimal part.

Fix formatting of numbers that can be floats when creating filenames and
dirnames: when transitioning to f-strings (commit ac6040f), it was
assumed that `sigma` and `nonrigid_grid_resolution` would be integers,
based on the existing formatting. However, these attributes can be set
to non-integer values. The pre-f-string transition solution, e.g.:
```
"iteration_%05d_sigma_%05d" % (self.total_iterations, self.sigma)
```

trimmed the decimal part, and thus, for e.g. `total_iterations` 20 and
`sigma` 7.5, it would produce:
```
'iteration_00020_sigma_00007'
```

With the introduction of f-strings, i.e.
```
f"iteration_{self.total_iterations:05d}_sigma_{self.sigma:05d}"
```

this would produce an error, since the `sigma` floating point value
cannot be formatted as an integer:
```
{ValueError}ValueError("Unknown format code 'd' for object of type
'float'")
```

This patch set fixes the error by adopting the previous behavior: it
trims the decimal part.
jhlegarreta referenced this pull request Jun 28, 2024
Prefer using f-strings.
@AlbertoImg
Copy link

Thanks @jhlegarreta for looking at my question!
I am not sure whether int(sigma) is the best option, the .5 in 7.5 is information needed for the report. In my local version, I changed d to f, and now the folder / files names have a "." in all the cases where sigma appears but still with the decimal info.

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jun 28, 2024

I am not sure whether int(sigma) is the best option, the .5 in 7.5 is information needed for the report. In my local version, I changed d to f, and now the folder / files names have a "." in all the cases where sigma appears but still with the decimal info.

I know it is not the best solution, but it is the behavior that it had before... You will agree with me that having a period in the filename/dirname is never a good solution. Happy to consider other solutions if you come up with any.

@jhlegarreta
Copy link
Contributor Author

@AlbertoImg I am merging this: the current workaround is better than a failure. As said, better strategies can be considered as new PRs.

@jhlegarreta jhlegarreta merged commit 39a5475 into SlicerDMRI:master Jun 28, 2024
9 checks passed
@jhlegarreta jhlegarreta deleted the FixFloatingPointFormatting branch June 28, 2024 23:25
@AlbertoImg
Copy link

A workaround could be to use "f" instead of "d" in the formatting, and later on replacing the dot in the dirname string:
For example:
dirname = f"iteration_{self.total_iterations:05d}sigma{self.sigma:03d}grid{self.nonrigid_grid_resolution:03d}"
dirname = dirname.replace(".","_")

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Jun 30, 2024

@AlbertoImg Please, check the statement: the strings still have the integer specifier. I had already thought about the solution you proposed, but discarded it because the result would not be consistent with what you get when using only integers, e.g.:

total_iterations = 10
sigma = 0.75
nonrigid_grid_resolution = 0.02

dirname = f"iteration_{total_iterations:05d}sigma{sigma:03f}grid{nonrigid_grid_resolution:03f}"
dirname = dirname.replace(".","_")

You get iteration_00010sigma0_750000grid0_020000. As you see, the floating point numbers are appended with zeros, whereas the integer is prepended with them: i.e. would that be preferred over iteration_00010sigma_00075grid_00020? It's perfectly fine from the point of view of the programming and certainly does not lead to confusion as opposed to iteration_00010sigma_00075grid_00020 (i.e. is sigma 75 vs 0.75 or the resolution 20 instead of 0.2?), but I am not sure about the downstream consequences, if any.

@AlbertoImg
Copy link

AlbertoImg commented Jun 30, 2024

Here "iteration_00010sigma_00075grid_00020", maybe confusing when comparing the format of the iterations and the others, but it is just my opinion.
Another idea:
dirname = f"iteration _ {total_iterations:04d} _ sigma _ {sigma:.2f} _ grid _ {nonrigid_grid_resolution:.2f}" # (spaces between "_" added just for visualization in this conversation)
dirname = dirname.replace(".","p")

'iteration_0010_sigma_0p75_grid_0p02'

@AlbertoImg
Copy link

AlbertoImg commented Jul 5, 2024

FYI: congeal_multisubject.py also has same issues. Thanks! Line 149 e.g.

@jhlegarreta
Copy link
Contributor Author

FYI: congeal_multisubject.py also has same issues. Thanks! Line 149 e.g.

Can you be more specific? The fix for the issue reported in ac6040f#r143635819 for whitematteranalysis/congeal_to_atlas.py was made extensive to whitematteranalysis/congeal_multisubject.py in this PR, and line 149 on that file does not point to a statement related to this PR:

meanabs = np.mean(np.abs(transforms_array), 0)

@AlbertoImg
Copy link

The line number was wrong. I am sorry. The line were 479 and 491
479: register.process_id_string = f"subject{subject_idx:05d}iteration{iteration_count:05d}sigma{sigma:03d}"
491: register.process_id_string = f"subject{subject_idx:05d}iteration{iteration_count:05d}sigma{sigma:03d}grid{grid_resolution:03d}"

@jhlegarreta
Copy link
Contributor Author

OK. Watch the underscores and markdown highlighting. PR #236.

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