Conversation
Replaced databroker imports with bluesky-tiled-plugins
Added alternative method to load scan in case databroker, etc. does not work and only a container of UIDs is available
Fixed package name
pbeaucage
left a comment
pbeaucage
left a comment
There was a problem hiding this comment.
See individual comments for changes needed.
src/PyHyperScattering/SST1RSoXSDB.py
Outdated
| catalogString = str(self.c) | ||
| if "Catalog" in catalogString: run = self.c[int(run)] | ||
| if "Container" in catalogString: ## Alternative method to load scan in case databroker, etc. does not work and only a container of UIDs is available |
There was a problem hiding this comment.
Please use an actual form of type checking if you are going to do this, not just casting to a string... isinstance() would be the most appropriate way, see line 573 of this file.
There was a problem hiding this comment.
I'm not sure that this actually gets you as far as you'd think, by the way. If you don't have the Bluesky tiled types you won't have a bunch of convenience methods that loadRun uses later (i.e, the resulting object won't be a BlueskyRun but some other native-tiled type, probably a DatasetClient) so that will fail. Probably best to just fail early if dependencies aren't installed/importable.
There was a problem hiding this comment.
I see. I mostly had done this for NEXAFS reduction, but not RSoXS, so I have reverted that change to be safe.
src/PyHyperScattering/SST1RSoXSDB.py
Outdated
| import tiled | ||
| import dask | ||
| from databroker.queries import RawMongo, Key, FullText, Contains, Regex | ||
| from bluesky_tiled_plugins.queries import RawMongo, Key, FullText, Contains, Regex |
There was a problem hiding this comment.
This will, of course, break old installs or installs that don't force the dependencies to update. For now, I'd use a try/except to support both import sources.
pyproject.toml
Outdated
| "databroker[all]>=2.0.0b10", | ||
| "bluesky-tiled-plugins", | ||
| "bottleneck" | ||
| ## TODO: tiled, databroker, and bottleneck probably can be deleted for data from December 2024 onwards, but need to check if this harms backwards compatibility |
There was a problem hiding this comment.
Proper place for this kind of future reminder is an issue, not a comment in a file.
src/PyHyperScattering/SST1RSoXSDB.py
Outdated
| catalogString = str(self.c) | ||
| if "Catalog" in catalogString: run = self.c[int(run)] | ||
| if "Container" in catalogString: ## Alternative method to load scan in case databroker, etc. does not work and only a container of UIDs is available | ||
| ScanUID = np.array(self.c.search(Key("scan_id") == int(run)))[0] |
There was a problem hiding this comment.
Few things here:
- no need to cast
runto int because this whole block only runsif isinstance(run,int) - I'm not sure this needs to be done via a
np.array, in general this kind of casting is not best practice.. doesn'tc.searchreturn aContainer? I didn't even realize Containers could be cast to arrays. Better route would bec.search(...).keys()[0]which iirc should give you a string.
There was a problem hiding this comment.
Makes sense, and I'll keep an eye for other instances. For now, I have reverted this change, as it might cause more harm than fix.
|
As a general suggestion, it's best practice to both use a descriptive PR title and to delete the default name for a commit so the top line message is useful, i.e, "change import source for Tiled types" not "Update SST1RSoXSDB.py". |
PriyankaKetkarBNL
changed the title
The alternative way to load from UIDs might conflict with downstream workflows.
|
I have made updates based on the suggested changes, and I also tested the same blocks of code detailed in Issue #171. Let me know if you think anything else is needed before merging. |
2 participants
Addresses issue #171