-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Construction #119
base: main
Are you sure you want to change the base?
Construction #119
Conversation
feat: use eccentricity instead of minor_length in detection (line 52, 117) fix: object shape bug when using min_sep (line 71) fix: changed to softer constraint of amt of sources in ComputeTransformXYShift (could be made variable?) feat: added automatic metadata and header to stack (from last image used for stack) because of issue with querying the stack before fix: minor fixes with slicing and shapes in fluxes (line 126, line 297) feat: masking, as image attribute, used in DAOFindStars and AperturePhotometry
Removed a block i modified for my own purpose. Made consistent naming for the eccentricity bound feature.
Forgot to adapt the name variable in the condition
Thanks for this PR @schackey 🙏🏼 these are important changes. I am happy to merge once we are set on the few comments I've made. |
@@ -57,6 +57,9 @@ class Image: | |||
|
|||
header: Optional[Header] = None | |||
"""FITS header associated with the image (optional)""" | |||
|
|||
mask: Optional[np.ndarray] = None | |||
"""Mask (commonly used for bad pixels) that is written in the CleanBadPixels block""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would omit the reference to block here, this mask will be quite general and changed by many other instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the mask should be initialized to a sensitive default I think
@@ -38,8 +38,8 @@ def __init__( | |||
minimum separation in pixels from one source to the other. Between two sources, greater ADU is kept, by default None | |||
min_area : float, optional | |||
minimum area in pixels of the sources to detect, by default 0 | |||
minor_length : float, optional | |||
minimum length of semi-major axis of sources to detect, by default 0 | |||
eccentricity_bound : int, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect a tuple specifying the bounds of the eccentricity here, such that sources are better separated depending on their shape (with a single upper bound only, point sources will also fall into traces). What do you think?
@@ -175,7 +175,7 @@ def run(self, image): | |||
|
|||
|
|||
class PointSourceDetection(_SourceDetection): | |||
def __init__(self, unit_euler=False, min_area=3, minor_length=2, **kwargs): | |||
def __init__(self, unit_euler=False, min_area=3, eccentricity_bound=2, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be that eccentricity is between 0 and 1, isn't it? This applies to the rest of the changes
@@ -40,7 +40,7 @@ def run(self, image: Image): | |||
|
|||
apertures = [image.sources.apertures(r) for r in radii] | |||
aperture_fluxes = np.array( | |||
[aperture_photometry(image.data, a)["aperture_sum"].data for a in apertures] | |||
[aperture_photometry(image.data, a, mask=image.mask)["aperture_sum"].data for a in apertures] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would anticipate that other blocks might modify the mask in different ways , such as 8 bit integers to specify the kind of bad pixel (cosmics, dead ... etc). For this reason it would be good to first cast the mask to a boolean before using it in aperture_photometry
Hi @schackey, should I take over this one? |
1
feat: use eccentricity instead of minor_length in detection (line 52, 117)
2
fix: object shape bug when using min_sep (line 71)
3
fix: changed to softer constraint of amt of sources in ComputeTransformXYShift (could be made variable?)
4
feat: added automatic metadata and header to stack (from last image used for stack) because of issue with querying the stack before
5
fix: minor fixes with slicing and shapes in fluxes (line 126, line 297)
6
feat: masking, as image attribute, used in DAOFindStars and AperturePhotometry
Currently, the mask is not set anywhere. I would like to discuss this. At first, I had it set for every image in the CleanBadPixels block to be set to the computed(or imposed) bad pixel map. However, I feel like it might be advantageous to leave more flexibility, maybe by adding a block called 'MaskImage', that takes a mask as an argument and writes it to the mask attribute of every image that passes through in the sequence. This way, one can use all different kinds of masks, instead of just bad pixel masks. Furthermore, one could leave out the CleanBadPixels block altogether (which currently automatically interpolates images when applied) in this way and still be able to mask nonetheless.
What are your thoughts?