search

bluesky / suitcase-tiff

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

Creating the `export(...)` function and adding tests #1

Closed awalter-bnl closed 5 years ago

awalter-bnl commented 5 years ago

This PR adds the suitcase.tiff.export(...) function following the new standard. It also includes tests. It will rely on the event model PR https://github.com/NSLS-II/event-model/pull/36 to run and for the tests to pass.

testing involves full 'round trip' testing on events, bulk_events and event_page documents.

awalter-bnl commented 5 years ago

Still working on the check that every field contains 2D data.

awalter-bnl commented 5 years ago

added the check of 2D data. Once we have this reviewed and merged I will create a PR to suitcase-csv to bring it into line with this.

awalter-bnl commented 5 years ago

The latest push relies on a new repo suitcase-utils which I am about to set -up now. Hence I have added the WIP tag to the title of this PR.

awalter-bnl commented 5 years ago

@tacaswell This was changed to be labelled as a WIP which I clearly commented as well. It was not ready for review as it has not yet been tested, hence why there are a few items that are not yet finished.

awalter-bnl commented 5 years ago

I have removed the WIP tag as this is now a working version, but relies on PR#1 to NSLS-II/suitcase-utils. This now closes out issue #2. We should resolve issues #3 and #4 prior to merging I think.

awalter-bnl commented 5 years ago

The latest changes resolve issue #4 This is now a working version ready for review.

awalter-bnl commented 5 years ago

@tacaswell , @danielballan and I had discussed moving the file close part to inside the stop document in the serializer. On further thought I see an issue, we cannot guarantee that we will always see a stop document and so it is not an effective solution to ensuring that the files are cleaned up.

The try: ... finally: 'close files' in the export method does ensure that the files are closed properly so I think we should leave it as is.

danielballan commented 5 years ago

Good thinking. One of the goals is to minimize the amount of state exposed by Serializer -- having both artifacts and manager_artifacts is less than ideal, both from a "state is bad and we should expose as little as possible" point of view and the point of view of user confusion. ("What's the difference between an 'artifact' and a 'manager artifact'"? they may naturally ask)

Would something like this work?


def export(...):
    try:
        ....
    finally:
        serializer.close()

class Serializer:
    def __init__(self):
        # No 'manager_artifacts' attribute
        self.artifacts = self._manager.artifacts
        ...

    def close(self):
        for file in self._files:
            file.close()

Notice that the semantics of repeated calls to close() in Python are thus:

In [1]: f = open('/tmp/stuff', 'w')

In [2]: f.close()

In [3]: f.close()  # idempotent

This gives the caller a way to ensure that the resources are cleaned up without exposing more internals.

awalter-bnl commented 5 years ago

Excellent idea @danielballan I have made the changes

awalter-bnl commented 5 years ago

Have made the changes requested by @danielballan

awalter-bnl commented 5 years ago

I think we are good to go

danielballan commented 5 years ago

I think this is good to go modulo discussion around tests in https://github.com/NSLS-II/suitcase-utils/pull/1 that may prompt changes to the tests here.

awalter-bnl commented 5 years ago

I have updated the structure of the dictionary to allow for multiple descriptors per stream. I am now working on updating the tests to test this scenario (the existing tests are still passing modulo PR#1 to suitcase-utils)

awalter-bnl commented 5 years ago

closing because it has been decided that we can't agree on a path forward