-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fitting widget refactor #3169
base: main
Are you sure you want to change the base?
Fitting widget refactor #3169
Conversation
updated for ver. 6.0 release source and with PR raised issues addressed.
macos-latest fails due to a know issue with binary signing. #3134 |
@rozyczko - there's conflicts to resolve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, a good refactor. I am seeing a couple of errors during testing that will need to be addressed before I can approve.
# make sure the logic contains at least one element | ||
assert self._logic | ||
# logic connected to the currently shown data | ||
return self._logic[self.data_index] | ||
|
||
@property | ||
def data(self): | ||
def data(self) -> Union[Data1D, Data2D]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: The setter accepts lists, but the getter only returns single data objects?
# add polydisperse parameters if asked | ||
if self.isActive and self.poly_model.rowCount() > 0: | ||
for key, value in self.poly_params.items(): | ||
model.setParam(key, value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After switching from core_multi_shell
with polydispersity enabled to sphere
, I get the following error when I try to calculate the model. This does not happen in v6.0:
ValueError: Model does not contain parameter thickness1.width
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All issues addressed (+ some more found out during testing)
Description
This is the refactoring effort, the same as in #2651 but reparented to the current
main
.All issues raised by @krzywon in the original PR have been addressed.
Additionally, the files involved in the refactor were "improved" by
ruff
(runningruff check
)This is part 1 of the refactoring, focused on the two biggest and most independent components.
Next, I will attempt to refactor out constraints to a separate module.
Review Checklist:
[if using the editor, use
[x]
in place of[ ]
to check a box]Documentation (check at least one)
Installers
Licencing (untick if necessary)