Model fitting: re-estimate model parameters #1952

kecnry · 2022-12-29T13:52:46Z

Description

This pull request implements a plugin API and UI to re-estimate the initial values of free model parameters for individual or all model components.

Screen.Recording.2022-12-29.at.8.53.51.AM.mov

TODO:

rebase onto main once model fitting tweaks and cleanup #1947 is merged (currently includes that diff as well)

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-12-29T14:04:46Z

Codecov Report

Base: 91.79% // Head: 91.79% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (efdae47) compared to base (3e92619).
Patch coverage: 92.68% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1952   +/-   ##
=======================================
  Coverage   91.79%   91.79%           
=======================================
  Files         140      140           
  Lines       14941    14978   +37     
=======================================
+ Hits        13715    13749   +34     
- Misses       1226     1229    +3

Impacted Files	Coverage Δ
...igs/default/plugins/model_fitting/model_fitting.py	`83.77% <86.36%> (-0.03%)`	⬇️
...default/plugins/model_fitting/tests/test_plugin.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

jdaviz/configs/default/plugins/model_fitting/tests/test_plugin.py

pllim

Code looks like code and the video is very convincing. Thanks!

pllim · 2022-12-30T16:49:33Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

+        new_model['parameters'] = [fixed_params.get(p['name'], p) for p in new_model['parameters']]
+
+        # reset traitlet so vue picks up on changes by adding (and then removing) an empty entry,
+        # this avoids resetting the open state of the accordion in the UI.


I'd love to know of a better way to do this... or requesting/implementing traitlet.force_update() upstream...

Maybe @mariobuikhuizen can advise but this shouldn't block merge. Just amazed you figured out this is how you make it do what you want. To me, it is like eating pizza with chopsticks. 😸

Traitlets can't discover in-place changes to a list or dict, so you always need to use methods that produce a new instance. You could consider using:

self.component_models = self.component_models[:model_index] + [new_model] + self.component_models[model_index + 1:]

Or you could use self.send_state("component_models"), which is similar to the envisioned .force_update() function.

I didn't know about send_state, that seems to do the trick and reads more clearly to me than always rebuilding from scratch, thanks!

pllim · 2022-12-30T16:50:48Z

Although, do you want to explain this new feature in the user doc?

kecnry · 2022-12-30T16:55:09Z

Although, do you want to explain this new feature in the user doc?

The model fitting docs already says "Model Parameters are automatically initialized with a guess. These starting values can be edited by the user. They may also be fixed by selecting the checkbox, so that they are not fit or changed by the model fitting." We could add a little here ("... with a guess from the selected data/subsets. These starting values can be edited by the user or re-estimated by clicking a button..."), but I also am always weary of going in tangents for advanced functionality that is self-discoverable in the UI.

rosteen · 2022-12-30T19:56:02Z

If I click "re-estimate" under one of the model components it works, but if I click the button above the component list I get an error (this is in Cubeviz):

File ~/projects/jdaviz/jdaviz/configs/default/plugins/model_fitting/model_fitting.py:608, in ModelFitting.reestimate_model_parameters(self, model_component_label)
    607 try:
--> 608     model_index, model_comp = [(i, x) for i, x in enumerate(self.component_models)
    609                                if x["id"] == model_component_label][0]
    610 except IndexError:

IndexError: list index out of range

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
File ~/opt/anaconda3/envs/viz_dev/lib/python3.10/site-packages/ipyvue/VueTemplateWidget.py:60, in Events._handle_event(self, _, content, buffers)
     58     getattr(self, "vue_" + event)(data, buffers)
     59 else:
---> 60     getattr(self, "vue_" + event)(data)

File ~/projects/jdaviz/jdaviz/configs/default/plugins/model_fitting/model_fitting.py:590, in ModelFitting.vue_reestimate_model_parameters(self, model_component_label, **kwargs)
    589 def vue_reestimate_model_parameters(self, model_component_label=None, **kwargs):
--> 590     self.reestimate_model_parameters(model_component_label=model_component_label)

File ~/projects/jdaviz/jdaviz/configs/default/plugins/model_fitting/model_fitting.py:611, in ModelFitting.reestimate_model_parameters(self, model_component_label)
    608     model_index, model_comp = [(i, x) for i, x in enumerate(self.component_models)
    609                                if x["id"] == model_component_label][0]
    610 except IndexError:
--> 611     raise ValueError(f"'{model_component_label}' is not a label of an existing model component")  # noqa
    613 # store user-fixed parameters so we can revert after re-initializing
    614 fixed_params = {p['name']: p for p in model_comp['parameters'] if p['fixed']}

ValueError: '{'altKey': False, 'ctrlKey': False, 'metaKey': False, 'shiftKey': False, 'offsetX': 116, 'offsetY': 14, 'clientX': 1127, 'clientY': 564, 'pageX': 1127, 'pageY': 564, 'screenX': 1860, 'screenY': 679, 'x': 1127, 'y': 564}' is not a label of an existing model component

kecnry · 2022-12-30T21:03:08Z

@rosteen - should be fixed now, thanks!

rosteen · 2022-12-30T21:13:43Z

Is it known that this doesn't respect spatial subsets in Cubeviz? You can select a small spatial subset such that the collapsed spectrum's flux is much smaller than that for the full cube, and see that, for example, a constant component's estimate is based on the full cube collapse even if you select that spatial subset.

kecnry · 2023-01-03T14:15:25Z

Is it known that this doesn't respect spatial subsets in Cubeviz?

This seems to have been broken by #1947... @bmorris3 and I are investigating now and I'll see if we can get the fix in this PR or if it makes more sense to address separately. Thanks for testing that case! 😬

EDIT: fixed in #1954. Hopefully we can get that tested/reviewed/merged and then I'll rebase this PR.

rosteen · 2023-01-03T14:34:33Z

Is it known that this doesn't respect spatial subsets in Cubeviz?

This seems to have been broken by #1947... @bmorris3 and I are investigating now and I'll see if we can get the fix in this PR or if it makes more sense to address separately. Thanks for testing that case! 😬

Well, sorry I didn't catch it there, but good we caught it now I guess!

Co-authored-by: P. L. Lim <[email protected]>

see spacetelescope#1954

kecnry · 2023-01-03T15:13:58Z

Re-estimation now works with spatial subsets in cubeviz (note that the estimation is done on the 1d spectrum even if doing a cube fit, which I think is the expected behavior):

Screen.Recording.2023-01-03.at.10.12.31.AM.mov

rosteen · 2023-01-03T15:50:23Z

jdaviz/configs/default/plugins/model_fitting/model_fitting.py

+
+        self.component_models[model_index] = new_model
+        # length hasn't changed, so we need to force the traitlet to update
+        self.send_state("component_models")


Ooh, that's good to know about. There might be a few other places we can simplify things with this.

rosteen

Looks good now, all subset combinations are working like I'd expect. Thanks!

kecnry added the plugin Label for plugins common to multiple configurations label Dec 29, 2022

kecnry added this to the 3.2 milestone Dec 29, 2022

kecnry force-pushed the model-fitting-reestimate branch 2 times, most recently from 8ebc617 to 0eeb211 Compare December 29, 2022 13:55

kecnry force-pushed the model-fitting-reestimate branch 2 times, most recently from 49059a6 to a63c2c7 Compare December 29, 2022 17:44

kecnry marked this pull request as ready for review December 29, 2022 17:53

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino, pllim and bmorris3 as code owners December 29, 2022 17:53

pllim reviewed Dec 30, 2022

View reviewed changes

jdaviz/configs/default/plugins/model_fitting/model_fitting.py Outdated Show resolved Hide resolved

pllim reviewed Dec 30, 2022

View reviewed changes

jdaviz/configs/default/plugins/model_fitting/tests/test_plugin.py Outdated Show resolved Hide resolved

kecnry force-pushed the model-fitting-reestimate branch 2 times, most recently from 9c8228a to d952c40 Compare December 30, 2022 16:44

pllim approved these changes Dec 30, 2022

View reviewed changes

pllim added the Ready for final review label Dec 30, 2022

kecnry force-pushed the model-fitting-reestimate branch from 46c2c49 to ee8b240 Compare December 30, 2022 21:03

kecnry mentioned this pull request Jan 3, 2023

fix 1d spectrum fitting for spatial-subset in cubeviz #1954

Merged

9 tasks

API/UI to reestimate model parameter initial values

c4cdb87

kecnry and others added 4 commits January 3, 2023 10:07

Apply suggestions from code review

94a70f1

Co-authored-by: P. L. Lim <[email protected]>

fix re-estimate all button args

44475f4

simplify forced traitlet list update

9f9350e

re-initialize to account for spatial subset in cubeviz

efdae47

see spacetelescope#1954

kecnry force-pushed the model-fitting-reestimate branch from c9b6525 to efdae47 Compare January 3, 2023 15:11

rosteen reviewed Jan 3, 2023

View reviewed changes

rosteen approved these changes Jan 3, 2023

View reviewed changes

kecnry merged commit a2f3ce2 into spacetelescope:main Jan 3, 2023

kecnry deleted the model-fitting-reestimate branch January 3, 2023 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model fitting: re-estimate model parameters #1952

Model fitting: re-estimate model parameters #1952

kecnry commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading

pllim left a comment

pllim Dec 30, 2022

kecnry Dec 30, 2022

pllim Dec 30, 2022

mariobuikhuizen Jan 3, 2023

kecnry Jan 3, 2023

pllim commented Dec 30, 2022

kecnry commented Dec 30, 2022

rosteen commented Dec 30, 2022 •

edited

Loading

kecnry commented Dec 30, 2022

rosteen commented Dec 30, 2022

kecnry commented Jan 3, 2023 •

edited

Loading

rosteen commented Jan 3, 2023

kecnry commented Jan 3, 2023

rosteen Jan 3, 2023

rosteen left a comment

Model fitting: re-estimate model parameters #1952

Model fitting: re-estimate model parameters #1952

Conversation

kecnry commented Dec 29, 2022 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Dec 29, 2022 • edited Loading

Codecov Report

pllim left a comment

Choose a reason for hiding this comment

pllim Dec 30, 2022

Choose a reason for hiding this comment

kecnry Dec 30, 2022

Choose a reason for hiding this comment

pllim Dec 30, 2022

Choose a reason for hiding this comment

mariobuikhuizen Jan 3, 2023

Choose a reason for hiding this comment

kecnry Jan 3, 2023

Choose a reason for hiding this comment

pllim commented Dec 30, 2022

kecnry commented Dec 30, 2022

rosteen commented Dec 30, 2022 • edited Loading

kecnry commented Dec 30, 2022

rosteen commented Dec 30, 2022

kecnry commented Jan 3, 2023 • edited Loading

rosteen commented Jan 3, 2023

kecnry commented Jan 3, 2023

rosteen Jan 3, 2023

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

kecnry commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading

rosteen commented Dec 30, 2022 •

edited

Loading

kecnry commented Jan 3, 2023 •

edited

Loading