Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Construction #119

wants to merge 4 commits into from

Conversation

schackey
Copy link
Contributor

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?

schackey added 4 commits July 18, 2023 14:06
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
@lgrcia
Copy link
Owner

lgrcia commented Jul 19, 2023

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"""
Copy link
Owner

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

Copy link
Owner

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
Copy link
Owner

@lgrcia lgrcia Jul 19, 2023

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):
Copy link
Owner

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]
Copy link
Owner

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

@lgrcia
Copy link
Owner

lgrcia commented Oct 3, 2023

Hi @schackey, should I take over this one?

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.

2 participants