-
Notifications
You must be signed in to change notification settings - Fork 47
Support for SAXS fitting #3194
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
Support for SAXS fitting #3194
Conversation
…atic download in dev mode
|
Does periodictable have the formfactors needed by auSAXS? Could we add them if it doesn't? |
AUSAXS uses residue information to determine the number of hydrogens bound to every atom in the structure. With this, compound form factors (CH, CH2, NH, ...) can be used instead of the pure atomic ones (C, N, O, ..) for better accuracy. This can actually have a significant impact on the quality of the fit. I imagine such aggregated form factor tables are outside the scope of periodictable, which seems more focused towards individual atoms? |
|
Yeah, it is currently out of scope. |
|
In regards to your 'Comments and suggestions' header, I'd like to note that the issues around data structures are currently being addressed in the refactoring project, although its going to take some time before that project is ready. Specifically, each reader at the moment just has a |
krzywon
left a comment
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.
Code review only. I wasn't sure if this was in a state to test functionality yet.
| class ComputationType(Enum): | ||
| SANS_2D = 0, | ||
| SANS_1D = 1, | ||
| SANS_1D_BETA = 2, | ||
| SAXS = 3 |
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.
Suggestion: Use more of the Enum class (untested...)
class ComputationType(Enum):
SANS_2D = 0, False # This may need to be in parentheses
SANS_1D = 1, True
SANS_1D_BETA = 2, True
SAXS = 3, True
def __new__(cls, value, is_avg_type):
obj = object.__new__(cls)
obj._value_ = value
obj.is_avg_type = is_avg_type
return obj
@classmethod
def get_by_index(cls, value: int):
value = int(value)
for member in cls:
if member.value == value:
return member
return cls.SANS_2D # Random default value...
# This will allow you to remove the magic combo box index inferences you have in the GSC:
self.is_avg = (ComputationTypeself.get_by_index(cbOptionsCalc.currentIndex()).is_avg_type)
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.
I think this solves an issue which shouldn't be there. self.is_avg was a former way of determining the current state, which is now replaced by the cleaner ComputationType enum. As such, self.is_avg is now rendered unnecessary, and should be removed. I didn't want to do this here, as it'd be a larger refactoring of the GSC which I am not feeling super confident in doing.
| match self.cbOptionsCalc.currentIndex(): | ||
| case 0: | ||
| self.model.set_calculation_type(ComputationType.SANS_2D) | ||
| case 1: | ||
| self.model.set_calculation_type(ComputationType.SANS_1D) | ||
| case 2: | ||
| self.model.set_calculation_type(ComputationType.SANS_1D_BETA) | ||
| case 3: | ||
| self.model.set_calculation_type(ComputationType.SAXS) |
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.
Using my suggested enum, this match/case could be replaced with self.model.set_calculation_type(ComputationType.get_by_index(self.cbOptionsCalc.currentIndex()))
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.
I disagree. cbOptionsCalc.currentIndex() is the currently chosen option in GenericScatteringCalculator.ui, and I think all interactions / magic numbers associated with its state should therefore be contained inside the GenericScatteringCalculator.py.
|
Closing in favor of #3786 |
Description
I have implemented support for performing SAXS fits through the AUSAXS library. Note that this requires a newer version of AUSAXS which must manually be moved to the
src/sas/sascalc/calculator/ausaxs/libdirectory, available from here: https://github.com/SasView/AUSAXS/releases/tag/v4I have refactored how SasView hooks into my library. I think this new approach is far superior, and is also easier to extend in the future. Due to this change, we will need extensive testing to ensure the existing SANS hook is still working as expected.
I also made some changes to the GenericScatteringCalculator GUI manager since my SAXS calculation need some additional data which was not previously available:
self.typeenum variable to the GenSAS object, with four possible values corresponding to the currently selected calculation model:I use this variable to check what calculations are supposed to be run, instead of relying on various combinations of existing variables, as was previously done.
Comments and suggestions
As part of this PR, I had to dig quite deep into the internals of this part of SasView. I have a few comments and suggestions for future improvements:
Todo
libausaxsmust now be included since there is no fallback for SAXS calculations.libausaxs?Future additions
This is a list of additional features which could be useful to do in a future PR:
Documentation
Significant documentation of this new feature is required. I will add this later.
Installers
This could potentially affect the installers since I'll have to change which version of AUSAXS is being shipped with SasView. Testing should be performed for each platform.
Note: All GH artifacts are currently broken due to using an older version of AUSAXS!