Fitting widget refactor #2651
Fitting widget refactor #2651
Conversation
wpotrzebowski
added
Discuss At The Call
There was a problem hiding this comment.
This is a lot cleaner and makes FittingWidget a bit more accessible. I haven't combed through all of the code, but the majority of the additions in new locations look like a simple copy/paste from FittingWidget.
I would suggest looking at the work in #2647 to be sure the changes made there are also applied here since merging the two will be nearly impossible.
| @@ -1891,7 +1677,7 @@ def onFit(self): | |||
| return | |||
|
|
|||
| # keep local copy of kernel parameters, as they will change during the update | |||
| self.kernel_module_copy = copy.deepcopy(self.kernel_module) | |||
| self.logic.kernel_module_copy = copy.deepcopy(self.logic.kernel_module) | |||
There was a problem hiding this comment.
kernel_module_copy is still in FittingWidget, not FittingLogic.
| self.setFocus() | ||
|
|
||
| self.iterateOverPolyModel(updateFittedValues) | ||
|
|
||
| def updateFullMagnetModel(self, param_dict): |
There was a problem hiding this comment.
Would this fit better in MagnetismWidget?
| updateDataSignal = QtCore.Signal() | ||
| iterateOverModelSignal = QtCore.Signal() | ||
|
|
||
| def __init__(self, parent=None, logic=None): |
| self._magnet_model = FittingUtilities.ToolTippedItemModel() | ||
| self.is2D = False | ||
| self.isActive = False | ||
| self.logic = logic |
There was a problem hiding this comment.
Would this be better served as self.parent.logic? The current implementation makes it seem like the magnetism and polydispersity widgets have their own logic elements.
| # Just create a simple param entry | ||
| self.addNameToPolyModel(i, param_name) | ||
|
|
||
| def addNameToPolyModel(self, i, param_name): |
There was a problem hiding this comment.
The i parameter was removed from this method in #2647 due to #2563, which is related to the distribution combo box of multiplicity models. You might want to do something similar here.
Short description of why this fails: In line 237, a polydispersity row is added to the end of all items, but then, in row 244, the combo box is added to the ith row, but i only goes up to the number of parameters, not accounting for n_shells and the number of polydisperse parameters in each shell.
| if param_name != param.name: | ||
| return | ||
| # Modify the param value | ||
| self._model_model.blockSignals(True) | ||
| if self.has_error_column: | ||
| # err column changes the indexing | ||
| self._model_model.item(row, 0).child(0).child(0,5).setText(combo_string) | ||
| else: | ||
| self._model_model.item(row, 0).child(0).child(0,4).setText(combo_string) |
There was a problem hiding this comment.
param and combo_string and not defined in this context.
wpotrzebowski
added
Discuss At The Call
butlerpd
added
Discuss At The Call
|
Should this be part of a 6.0 release? or is it a bridge too far? |
butlerpd
added
the
Discuss At The Call
|
Could be done as 6.1 so not a top priority but would be nice to have |
Seeing how close we get with 6.0, I'd prefer to postpone it to 6.1. Maybe merge to |
wpotrzebowski
removed
the
Discuss At The Call
|
I will have to redo parts of it seeing how the original |
Description
This is a code maintenance/upkeep commit.
FittingWidget.py grew too big and needed to be refactored.
This PR addresses polydispersity and magnetism tabs/logic.
Fixes #
How Has This Been Tested?
Local dev build on Win10
Review Checklist (please remove items if they don't apply):