Skip to content

Conversation

@noemifrisina
Copy link
Contributor

Add a cli for ssx fixed target collections

@noemifrisina noemifrisina requested a review from keeble February 21, 2023 13:54
@noemifrisina
Copy link
Contributor Author

As things stand, the current code will correctly bin the first image but return empty ones after that. The issue seems to come from the use of valid_events with arrays instead of a single value for start ans stop.

Going to try and find a solution.

@graeme-winter
Copy link

Having a very quick look at the output, rather than the change set. @noemifrisina you were concerned that the same frame was simply repeated 400 times: this is not teh case

image

Derived from:

import numpy
import hdf5plugin
import h5py
f = h5py.File(filename, "r")
d = f["/entry/data/data"]
for j in range(400):
  print(j, numpy.sum(d[j,:,:]))

@graeme-winter
Copy link

Looking at the very constant spot count per image output I am going to guess there are some unmasked bad pixels here.

@noemifrisina
Copy link
Contributor Author

Thanks for checking.
About the almost constant spot count: unmasked pixels are entirely possible. But as this is a corner block, it might also be that there was nothing in most windows.
Btw, right now there's no mask in that dataset (as it's from last year, the file needs to be copied in the same directory as the binned images for it to work - newer collections won't have this issue).

@graeme-winter
Copy link

OK, lack of mask would probably do it, thank you
Screenshot 2023-05-11 at 14 39 03

Copy link

@keeble keeble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not tested it (It looked as though there's already testing afoot) so this is just a drive-by code review.

Generally looks pretty good to me, a couple of small inline comments added which may just be my misunderstanding.

Only overall thought (which applies to existing code, not just this PR) would be that a lot of the logic here sits within the cli function itself, meaning it's not necessarily that easy for someone else to use the code from elsewhere. I guess the purpose of this package is to provide cli only? Probably a separate PR in any case :-)

@noemifrisina noemifrisina merged commit f72a1a8 into main Jun 21, 2023
@noemifrisina noemifrisina deleted the fixed-target-ssx branch June 21, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants