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

Small changes in fitting.py and models.py #58

Merged
merged 5 commits into from
Jan 15, 2015
Merged

Conversation

srodney
Copy link
Contributor

@srodney srodney commented Jan 13, 2015

Added checkeffects to bandoverlap and propagating min chi2 parameter values into best-fit model objects.

bandoverlap_checkeffects_test_ipynb

 * in fit_lc, set the output model parameters to the
   values that minimize the chi2
 * bandoverlap was not accounting for added effects (e.g. dust), so that
   it could erroneously return True for a band that extends further than
   a model effect does.
 * * now set checkeffects=True to include dust, etc. when evaluating
   max/min wavelengths
Merging changes from original repository with srodney fork before
issuing a new PR
@kbarbary
Copy link
Member

Bug fix in fitting.py looks good. I think we never noticed this omission because the chisq function sets the model parameters as the minimization runs. So at the end of the minimization, the model parameters end up being pretty close to the optimal values. I guess you found cases where the model parameters were close (but not identical) to those reported in the result object?

The intent with model.bandoverlap was to always check effects (see #21) so I think the default behavior is just a bug. I'll take a closer look to make sure.

@kbarbary
Copy link
Member

With Model.bandoverlap I think it would be sufficient to change self._source.minwave() * (1. + z) to self.minwave() and similarly for maxwave().

@kbarbary
Copy link
Member

I meant Model.bandoverlap not Model.bandflux in the previous comment (now edited).

@srodney
Copy link
Contributor Author

srodney commented Jan 14, 2015

"I guess you found cases where the model parameters were close (but not identical) to those reported in the result object?"

Yes... though I don't have a good test-case ready at hand.

"With Model.bandoverlap I think it would be sufficient to change self._source.minwave() * (1. + z) to self.minwave() and similarly for maxwave()."

Yep. I think you're right. Fixed in the latest commit.

@kbarbary
Copy link
Member

Looks good.

As a side note, instead of doing git merge upstream/master to merge the latest commits to master into your branch, you can do git rebase upstream/master. The two methods are pretty similar in that you will have to resolve any conflicts that git cannot automatically resolve. The difference is that git rebase "rewrites" your commits so it looks like you started from where upstream/master is. The benefit being that the history is more linear and there aren't a lot of extra merge commits.

kbarbary added a commit that referenced this pull request Jan 15, 2015
Small changes in fitting.py and models.py
@kbarbary kbarbary merged commit 28c9ef1 into sncosmo:master Jan 15, 2015
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