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

Bugfix sed #126

Merged
merged 5 commits into from
Jul 24, 2019
Merged

Bugfix sed #126

merged 5 commits into from
Jul 24, 2019

Conversation

pmelchior
Copy link
Owner

SED initialization code was not robust for negative/zero flux in some bands. As such, such an SED is not a problem. I therefore changed the behavior from raising an exception to logging a warning.

I also, moved the SED correction (for PSF width variations) from get_pixel_sed to the relevant init_ functions because this is where it's needed. It's confusing otherwise.

@pmelchior pmelchior requested a review from fred3m July 17, 2019 20:33
@pmelchior pmelchior added the bug label Jul 17, 2019
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 the idea of only using positive values of the sed but I have a design question and a question about a negativity check in my comments below.

docs/quickstart.ipynb Outdated Show resolved Hide resolved
scarlet/source.py Outdated Show resolved Hide resolved
scarlet/source.py Show resolved Hide resolved
@pmelchior
Copy link
Owner Author

@fred3m , I've addressed all comments, and created a simpler implementation of MultiComponentSource that actually uses a common center for all components. You can merge if you are fine with it.

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.

Thanks for making the changes. Everything looks good except that I didn't quite like the change in MulitComponentSource where you removed the update function from the individual components and instead put them in the parents update function. While it works fine this way I think that it goes against our design pattern of having child components with update functions and a parent that just updates the children. Feel free to push back if you feel that I'm being too rigid.

scarlet/source.py Show resolved Hide resolved
scarlet/component.py Show resolved Hide resolved
@fred3m fred3m merged commit 767fc42 into master Jul 24, 2019
@fred3m fred3m deleted the bugfix_sed branch July 24, 2019 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants