-
Notifications
You must be signed in to change notification settings - Fork 31
Generalize the PETS input writer and remove legacy/unused code #134
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
Conversation
This is the way 🚀 That would probably be me 😅 Thanks for completely over-engineering PETS2 generation, looking forward to reviewing this one! Also, please add some basic tests for this new function, and documentation on how to customize the PETS data output. Feel free to drop 3.7/3.8 as discussed in #130 |
Sure thing, I will add a few tests to assert the input is created properly as the config is changed. Nota bene I wrote this in such a way that without any affixes the input is identical with previous one. Given the freedom to drop 3.7/3.8, either no longer supported as of October 2024 (link), I will also move the csv to a separate file, but I'll try to leave most of the other code as is, other than maybe optimizing it with a few walruses 🦭 :). |
In addition to the main content of this PR as already covered above, after the discussion I:
IMO the new |
@stefsmeets Any specific comments or suggestions? Would you maybe prefer different testing, or maybe split Python version shift and PETS input into separate PRs? Sorry to urge you, but I have another PR lined up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for adding the tests, looks good to me. I added some small comments. Have a look and see what you think. Thanks for picking this up!
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
I addressed the suggestions; to summarize:
|
Looks good to me, feel free to merge :-) |
Context
This week I decided to systematize creating the pets input file. In the current version of the code, the file is created in a brute-force way, by printing fixed lines into a file. The lines are completely hardcoded and, in contrast to e.g. very voluminous albeit rather primitive XDS file creation, customization options are very limited. Produced file can not be altered in any way by the user and it may be even illegal because there is no mechanism looking for duplicate keys.
The issue with making pets input is exacerbated when looking at the unused function
write_pets2_inp
. Apparently, at some point someone had a need to create slightly different pets input file, so in order to get to their goal while preserving the backwards-compatibility, they just copied existing functions and changed a few lines! This approach, seemingly popular for XDS (?) can in my opinion be excused at most on an experimental branch, notmain
!In this PR, I suggest adding a new mechanism for creating PETS input files that utilizes objects in newly-added
PETS_input_factory.py
, in particular thePetsInputFactory
itself. Instead of printing new lines, it is now sufficient toadd
them one by one to the factory, and thencompile
the result, which will produce the final file. In addition to being more readable at the higher level i.e. in theImgConversion
class, it can do things that the previous implementation could not:config.camera.pets_prefix
stores information that goes before = takes precedence over the generic information;config.camera.pets_suffix
stores information that goes after = can be overwritten by the generic information.Both
pets_prefix
andpets_suffix
can include Python3 format-style replacement fields i.e.{attrubute}
. When compiling the PETS input, these will be replaced with class attributes ofImgConversion
, if present, and – notably – ignored otherwise. With this combination, the user has significant agency over the shape of their PETS input file produced. For the need of my setup and experiments, I have added the following fields to myserval.yaml
file:Note that the suffix has several fields that will be filled when the input file is compiled:
{osc_angle:.3f}
,{headers[0][ImageExposureTime]:.6f}
,method
. See the final file below to see how they have been handled.Furthermore, the prefix contains two fields,
noiseparameters 4.2 0
andreflectionsize 8
, that the genetic factory sets asnoiseparameters 3.5 38
andreflectionsize 20
by default. Because the fields from the prefix precede the default lines, the latter are ignored and my final file has the values correct for my detector, producing this nice PETS input file below. Mind that with this degree of freedom, it should be no longer necessary to have multiple separate functions writing pets output, but if you need them for some reason, making them is now also much simpler (seeImgConversion:ImgConversion.write_pets_inp
).Minor changes
config.camera.pets_prefix
andpets_suffix
can be used to dynamically expand the.pts
made after an experiment.Effect on the code
ImgConversion
class by providing a general way to make pets input files.Note
If would be better to have the pets keyword specification stored in a separate
.csv
rather than hard-coded in a string; this would be best done usingimportlib
, which is unavailable in python 3.7 & 3.8 and I don't want to play with paths manually.