-
Notifications
You must be signed in to change notification settings - Fork 125
save pixel size settings in saveSystemConfiguration #372
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
base: main
Are you sure you want to change the base?
Conversation
|
The sad news is that I am quite uncertain about the current state of There is no unit test for this function (although it would be nice if there were), so yes, the current practice is just local testing. I'll take a quick look at the code and test it before merging. |
|
In MMStudio (which is effectively the reference implementation for config file writing), the affine transforms are only written when they are set. It would be best if we do the same here. But the situation is a little murky. If you set the affine transform for even one pixel size preset in Pixel Size Calibration, all presets now have an affine transform saved in the config file. The unset ones get [1 0 0 0 1 0] (i.e., identity transform), which would make sense if the affine transform is applied in addition to the conventional pixel size, but that is not how it works. Is there a deeper problem lurking here? @nicost. |
|
yeah, i noticed that too. For what it's worth, I recently re-implemented all of this logic in pymmcore-plus including the microscope model and writing the config file from the model so I could create a full config wizard GUI in pymmcore-widgets ... and there I also skip default affines as in MMStudio so it's not super critical that I have this change in mmCoreAndDevices. But, seems like a natural place for it to be the reference implementation :) |
This I agree with. |
My memory is vague. I believe that I though it safer to store the identity matrix by default, rather than nothing, or an all zero matrix. Probably no deep thought went into this.... |
|
I guess then I would be okay with merging something that behaves exactly the same way as the Java writer, for now. This for the cases: 1) no affine matrices set, 2) some presets have affine matrices set, and 3) all presets have affine matrices set (probably 2 and 3 are the same case). It would seem that any code that wants to use the affine transform needs to treat [1 0 0 0 1 0] as a special "null" value indicating the absence of an affine transform. This probably does not cause logical problems in practice, because the only case where [1 0 0 0 1 0] should be treated as an actual affine matrix is when the pixel size is 1.0 um/px, but then the result is the same as having no affine matrix. Whew. But it might be worth improving the Core API so that callers don't need to (know that they have to) manually check for the special value. |
| std::vector<double> affine = getPixelSizeAffineByID(configs[j].c_str()); | ||
| os << MM::g_CFGCommand_PixelSizeAffine << ',' << configs[j] << ','; | ||
| for (size_t i = 0; i < affine.size(); ++i) { | ||
| os << affine[i]; | ||
| if (i < affine.size() - 1) { | ||
| os << ','; | ||
| } | ||
| } | ||
| os << endl; |
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.
this is the main remaining question. cc @marktsuchida
This PR would add PixelSizeConfigs to the output of
saveSystemConfiguration(similar to MMStudio's output).@marktsuchida, haven't tested this locally, so let me know if there's any CI to confirm that stuff works, or if this repo just relies on local testing / visual inspection