bluesky / suitcase-tiff

http://nsls-ii.github.io/suitcase
Other
2 stars 5 forks source link

ENH: splitting the series and stack version into seperate modules #17

Closed awalter-bnl closed 5 years ago

awalter-bnl commented 5 years ago

This PR was prompted by @CJ-Wright requesting the ability to extract information from both descriptor and event docs as well as the start doc to format into the filename when a tiff 'series' is used (i.e. one file per image).

@danielballan was further concerned that for tiff 'stacks' (all images from a stream and field in the same 'tiff' file) the filename formatting API would be different than our other suitcases (csv, json_metadata) that have similar 'stacks'. Therefore he requested that we split the suitcase-tiff into 2 modules suitcase.tiff_series and suitcase.tiff_stack. They remain in the same repo so pip install suitcase-tiffwill install both modules.

Testing has also been split now so that each module has it's own set of tests which all pass locally.

awalter-bnl commented 5 years ago

Tests are passing except that codecov is complaining that we have reduced coverage to ~ 95%. I will leave it up to others to decide if I need to improve to get codecov to pass.

awalter-bnl commented 5 years ago

thanks @tacaswell for the comments, I have resolved some and will fix others today, however I have left one comment asking for more context as I am not sure what you mean.

awalter-bnl commented 5 years ago

The issue with the codecov tests is that it is checking the coverage of the *_version.py files. I attempted to have them ignored but that was unsucessful, suggestions welcome. All other files are close to 100% coverage.

danielballan commented 5 years ago

@awalter-bnl 454c9bc85ba01c8756074507e0f5f4fca29f009f ("FIX: remove _version.py files from codecov tests") configured codecov which publishes the results instead of coverage which collects the results. I have force-pushed to your branch to delete 454c9bc85ba01c8756074507e0f5f4fca29f009f and add be9c98f which I suggest is the proper fix.

Then, looking at the resultant coverage report:

 $ coverage report -m
Name                                     Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------------------------------
suitcase/tiff_series/__init__.py            35      0     10      0   100%
suitcase/tiff_series/tests/__init__.py       0      0      0      0   100%
suitcase/tiff_series/tests/conftest.py       3      0      0      0   100%
suitcase/tiff_series/tests/tests.py         80      4     52      1    93%   75-81, 74->75
suitcase/tiff_stack/__init__.py             58      2     18      2    95%   182, 216, 179->182, 215->216
suitcase/tiff_stack/tests/__init__.py        0      0      0      0   100%
suitcase/tiff_stack/tests/conftest.py        3      0      0      0   100%
suitcase/tiff_stack/tests/tests.py          13      0      2      0   100%
------------------------------------------------------------------------------------
TOTAL                                      192      6     82      3    95%

I noticed that some lines of the tests are not being run, specifically ---

suitcase/tiff_series/tests/tests.py         80      4     52      1    93%   75-81, 74->75

which looked suspicious. The next three commits address that. If this all looks right to you, we should get a third person to review.

awalter-bnl commented 5 years ago

Thanks @danielballan , much appreciated. I agree with the changes you made.

danielballan commented 5 years ago

I found a couple more small things on a second review and went ahead and pushed fixes. 1e8daf8 is my fault and should be fixed on all suitcases. 1c44c93 is just my opinion -- see multi-line commit message for details.

awalter-bnl commented 5 years ago

sorry @danielballan most of those where remnance from when I move the base class from tiff_series to tiff_stack which I should have caught. I see no problem with these changes.

CJ-Wright commented 5 years ago

Is there a good way to include hinted variables automatically? Its very common for users to want the values of these variables in their filenames so that they know which images match to which data points. We've done something like this with xpdAn which reduced a lot of burden producing file templates for each possible scan.

danielballan commented 5 years ago

I think that's a job for a higher layer, at least for now. The caller has access to the start document and can use it to decide what to pass into file_template.

awalter-bnl commented 5 years ago

I think I agree with @danielballan, auto-templating will be a beamline specific process and should be left to either a wrapper around these general versions or beamline specific versions. We want to avoid adding to many 'features' which then lead to complicated general purpose suitcases that aren't really useful for anyone.

danielballan commented 5 years ago

@CJ-Wright If you'd like to open an issue to continue discussion, suitcase-core would be a good place for general discussions that affect all suitcase formats.

awalter-bnl commented 5 years ago

Thanks @mrakitin @tacaswell and @CJ-Wright for the reviews and @danielballan for the commits. I have left some comments above.