Fill XDS_template from base instead of writing 4 almost-identical files#158
Conversation
stefsmeets
left a comment
There was a problem hiding this comment.
Nice! Lines changed: 30 additions & 356 deletions always makes me happy =)
The only thing I don't like in this PR is that it reads the file during runtime (basically copying the template from XDS_template.py->XDS_template.inp). It's almost always easier an less error prone to define the format as a string in Python. On top of that you get autocompletion and syntax highlighting for free.
I moved it once from INP -> .py, so this feels like a regression to me 😅
|
Found it: 4ffc225 |
|
I will not lie, the "+30 -356"-induced dopamine rush was a factor in taking this up ASAP :). I personally find storing files in external files more convenient and maintainable, and my main co-worker shares my opinion https://chatgpt.com/share/699330f1-abdc-800e-86ce-6e2998e8e22a. This might be because I do not have any specific highlighting enabled for strings, they are just all-plain green. Well, apart from slashes, which in string require escaping and thus appear orange |
This is fine, if it weren't for the fact that the INP file by itself does not work in XDS (after all, it contains python formatting strings). So it is a Python string you are storing in a INP file, so all you are doing is reading a Python string from a text file, which Python itself is also very good at =) Btw, please don't insult me by responding with some nonsense AI reply (tldr btw). I'm more than happy to review these PRs and sharing lessons learned, and if you rather go with your co-worker's 'opinion' (ai:dr), I'm not stopping you. But then please don't ask me for reviews anymore. |
|
Got it. I do find it quite useful to supplement my personal opinion with AI because it is essentially a statistical overview of all libraries existing in its learning set, but I won't bring it up if you prefer it. |
|
Given no further objection, I will merge this PR alongside #157 and include it in a new release 2.2.1 by Wednesday. I can move the string back to the file; the argument it is a Python f-string rather than a functional .inp is solid. |
When testing building as described in #156 and #157, my pipeline ran a bunch of simple sanity checks via a package
check-wheel-contents. One of the checks looked for and identifiedinstamatic/processing/XDS_template.pyandXDS_templateTPX.pyas two files with different filenames and identical content. Indeed, these files are identical, with furtherXDS_templateDM.pyandXDS_templateTVIPS.pydiffering by 2-3 lines. Thus, I suggest using a PartialFormatter introduced in #134 to merge the four files into one, partially format them based on the pipeline, and pass to the image converter to do the rest of the work.Before the application of the partial formatter, the base template class looks like this (excerpt):
After the partial formatter, some fields will be filled – only those present in the dictionary given. The fraction of the dictionary used here would be
{'max_cell_axis_error': '0.03', 'max_cell_angle_error': '2.0'}. Remaining{}fields stay unaffected without raising an exception, as the default python formatter would:Note
{starting_angle:0.4f}remains unfilled, ready to be filled byImgConversion.This PR has no impact on functionality whatsoever. It aims to improve maintainability only.