Added additional save features to SaveCroppedObjects

[callum-jpg]

Added the option to add input filenames to output crops and also the possibility to save output crops into nested folders.

[callum-jpg]

So for this I've hit a bit of a roadblock, namely trying to get the filename tests to work. I've included what I've tried so far in here. Basically, the error arises in response to the source_file_name_feature method used in SaveCroppedObjects, which is: AssertionError: Feature FileName_example for Image does not exist. I'm posting this here so that there may be light shed on what I'm missing.

The same method I'm using is also used in SaveImages, but test_saveimages does not cover this method.

[bethac07]

So, a couple of things here -

First of all, I missed that in the main module itself, we definitely need to bump the module version and add an upgrade_settings function for upgrading the version - apologies for missing that earlier!

Second of all, as currently written, we're not "[Adding] the option to add input filenames to output crops" (emphasis mine), we're actually enforcing it. 95% of the time, I think this is what users want, but it's ultimately a breaking change. As such, we either need to a) make it an option and/or b) we should hold it for CP5. In general, I'm nearly always in favor of making things optional, but I understand the need to sometimes stop the option-creep. If we do make it optional, I'm fine with having the default be True for newly added modules, but we want it to be "False" for upgraded modules in your upgrade_settings that you'll be adding.

Finally, RE: tests, I see what you were trying to do here @callum-jpg, but you were going at it in a harder way than you needed to - rather than forcing CellProfiler to load the data the way that you wanted, just making sure CellProfiler has the information it's going to need to do the thing you want to check is WAY easier, and ultimately, what we care about here - we don't actually want to test CellProfiler image loading here, we just want to test that it will make file names the way we want them to. Make sense? I removed them, because I think there are going to be easier ways to do what you want to do, and because there are still module decisions to make, but you can always get them back in the commit history.
The existing tests are failing, due to the fact that we've changed the way file names are constructed. I fixed just the first one (with some code I stole from the NamesAndTypes test), and it passes now. So depending on the decision on optional-ness, a couple of different things to possibly do here -

1. Decide this change is going to be mandatory, in which case, fix the rest of the failing tests according to the model of how I fixed the first one, and then just add yourself one more test for nested foldernames (which should be more clear to do after fixing the others)
2. Decide this change is going to be optional, add the code to the module to make it so, and then in the tests a) copy my "fix" of the first test as a new test that now checks this "optional" behavior, b) restore the first test to the way it previously was, and make sure it and the others have the appropriate options set (ie "use image file name" or whatever you call it = False) so that they follow the previous behavior and c) add any other tests you think are necessary - I think probably still just something for nested folders but up to you.

I threw some time on your calendar to walk through all of this later today, in case anything was unclear here!

[bethac07]

Per an offline conversation, we're going to change this behavior to be optional, and try to get it into 4.2.2. Should be simple from here!

[bethac07]

Me: "this module is already on revision 2, why didn't it have an upgrade_settings already?"
Also me:

[callum-jpg]

When running pytest on test_savecroppedobjects.py, test_run_images fails for the grayscale_volume and multichannel_volume images. However, running pytest tests/modules/test_savecroppedobjects.py::test_run_images does not lead to the same test failing. I'm not sure what is causing this. Can anyone else confirm if this happens on their side?

[bethac07]

I could validate the behavior - if you look at the order the tests were running, it will give you an initial clue why this was happening.

If I run just that one test -

If I run all the tests -

It gets even more clear when you add some print statements - here to the failing test and the nested test I've added prints for the name of the directory and whether it exists; also to the failing test, I've had it print the results of its glob, then an os.listdir of the directory

So based on all that, here is what was happening -
When its running all the tests using the 3 "standard" fixtures (the last 4 tests do NOT use the standard fixtures), rather than test-by-test going through all 3 fixtures, it fixture-by-fixture goes through all the tests that use those fixtures and then goes onto the next fixture.
Also, there are two ways tests can be set up - re-initializing the workspace and the module every time via a function run every time (this is typical in the older tests, where you see those "make_workspace" functions), OR you can re-initialize the module and the workspace every time in each test (you can see an example of this in the ExportToSpreadsheet tests) OR they can be using the conftest-provided-workspace and typically, sharing a copy of the module that's set at the top of the test file with "instance". The savecroppedobjects tests are using the last one.

The first fixture, since nested_save default is false, and because the test that expects nested_save to be off runs first, both tests pass fine. BUT, on the second and third fixtures, since the tests are sharing a copy of the module, you've now set nested_save is true when you ran the test for that, but you haven't explicitly re-set it BACK to false. So it's still set to true, and so the first test (whose asserts think nested_save will be off) fails. Adding an explicit set of that property back to false in whichever is the first module using those fixtures AFTER the nested_save test fixes the test.

I've done that, but also re-added-back the initial image test. I think this is probably now ready for review!

(Edited to add - so if I've said the module is shared across all tests, why DON'T the other tests fail, the ones using non-standard fixtures? It's because of the test_defaults test, which just re-sets all the module settings back to the defaults as part of it. If you removed THAT test, the three tests after using non-standard fixtures WOULD have failed)

[gnodar01]

Added the new file format to the help docs, but other than that looks good to me.