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

Fix compound model fit failure #276

Merged
merged 5 commits into from
Sep 10, 2020

Conversation

rosteen
Copy link
Collaborator

@rosteen rosteen commented Sep 9, 2020

Updating the displayed parameters after fitting a compound model was failing, due to specutils.QuantityModel not being subscriptable with the component model names. I left in the old parameter update code to be triggered if the resulting model isn't a QuantityModel (I don't know if this will actually happen ever, but better safe than sorry). Fixes a bug raised in #263 by @PatrickOgle that fell off my radar earlier in the year (TypeError: 'QuantityModel' object is not subscriptable).

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #276 into master will decrease coverage by 0.74%.
The diff coverage is 7.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   63.42%   62.67%   -0.75%     
==========================================
  Files          47       47              
  Lines        2614     2650      +36     
==========================================
+ Hits         1658     1661       +3     
- Misses        956      989      +33     
Impacted Files Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py 31.83% <7.89%> (-4.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ad287c...79c0be2. Read the comment docs.

@rosteen
Copy link
Collaborator Author

rosteen commented Sep 9, 2020

This needs a little more work before review, a case I didn't initially test is failing at the moment.

EDIT: now ready for review!

@rosteen rosteen force-pushed the compound-model-fix branch 2 times, most recently from 8544c3d to 8160233 Compare September 9, 2020 16:03
Copy link
Contributor

@javerbukh javerbukh left a comment

Choose a reason for hiding this comment

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

Looks good! Just remove the print statement and I think this is good to merge.

Copy link
Contributor

@PatrickOgle PatrickOgle left a comment

Choose a reason for hiding this comment

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

The bug appears to be fixed. However, I can't git a Gaussian1D + Linear1D model to give a reasonable
fit to a subset--a separate issue for me to look into.

@rosteen rosteen merged commit 5fa78f5 into spacetelescope:master Sep 10, 2020
@eteq
Copy link
Contributor

eteq commented Sep 10, 2020

@PatrickOgle - can you make a github issue about that specifically and link it here?

@rosteen rosteen deleted the compound-model-fix branch September 10, 2021 17:20
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.

5 participants