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

init with size of at least the PSF #234

Merged
merged 5 commits into from
Apr 6, 2021
Merged

init with size of at least the PSF #234

merged 5 commits into from
Apr 6, 2021

Conversation

pmelchior
Copy link
Owner

stabilize the morphology initialization at the faint/small side by making sure its at least as wide as the PSF

Copy link
Collaborator

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

I like it. It also satisfies Bob's (and likely other peoples) request that the box size can be set or fixed if desired (whether or not that is a good idea is another discussion).

@@ -189,9 +190,23 @@ def __init__(
# retain center as attribute
self.center = morphology.center

def init_morph(self, sky_coord, frame):
@staticmethod
def init_morph(frame, sky_coord, boxsize=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get why you made this a static message, but is there a reason that you changed the order of the arguments?

Copy link
Owner Author

Choose a reason for hiding this comment

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

same order as the constructor

@pmelchior
Copy link
Owner Author

I'd like to hold off with the merge until we tested Remy's random init idea because it can help in similar cases. I will play with it a bit.

@pmelchior
Copy link
Owner Author

Actually, re boxsize. It's not exposed at the level of the constructor. If this is desired functionality, I could add it.

@fred3m
Copy link
Collaborator

fred3m commented Feb 12, 2021

Yes, that would be preferred. I implemented a fix in LSST that added a config option and just switched off _resize for all of the morphologies, but the ability to turn it off (or specify a box size) would be great.

Copy link
Collaborator

@fred3m fred3m left a comment

Choose a reason for hiding this comment

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

Everything looks good except that you don't expose resize to the the sources, only the morphologies. So I would add a resize argument to ExtendedSource and the other source classes and pass that through to morphology, which already has a resize argument. Then this should be all set.

@pmelchior
Copy link
Owner Author

I had a slightly different philosophy when writing this. The things that need to be exposed are those that cannot be altered otherwise: initialization foremost. However, anyone can switch on/off both resizing and shifting now after the source has been created.

@fred3m
Copy link
Collaborator

fred3m commented Mar 11, 2021

Yes, but it's not trivial when given a list of sources and you don't know what type of source each one is. You have to iterate over all of the sources, check the source type, and descend down the components until you get to the morphology, and set resize to false. That procedure is also prone to break, perhaps unknowingly, when we change the source/morphology classes internally.

It's much simpler and less prone to hidden failure to be able to pass resize and shift to the init_all_sources method and have the models created in the way that you expect, where any breakage due to a change in API will be obvious.

@fred3m
Copy link
Collaborator

fred3m commented Apr 6, 2021

Looks good. It's interesting to see some of the changes in the residuals for the test dataset. Most of them look better, but some have strange color artifacts in the center. Do you know what's causing those?

@pmelchior
Copy link
Owner Author

I just checked the residuals of the validation data set. The overwhelming majority looks better now, I've only found one case where this branch looks worse than the main branch (and that was almost certainly a star). I think we are OK to proceed.

@pmelchior pmelchior merged commit f78a044 into master Apr 6, 2021
@pmelchior pmelchior deleted the psf-max-init branch April 6, 2021 21:35
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.

None yet

2 participants