-
Notifications
You must be signed in to change notification settings - Fork 14
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
Refactor the way analysis settings are set #110
base: v2.0.0
Are you sure you want to change the base?
Conversation
- Only allow specification through keyword arguments, use `from_settings` classmethod to use a settings object - Use __init_subclass__ to automatically determine the settings object type from the class signature - Use __init_subclass__ to specify class-level settings as arguments instead of attributes
The classmethod is now called |
…s' into crnh/v2.0.0/fix/analysis_settings
Todo at a later stage: add an example on how to iteratively update only one value of the settings class |
types.NoneType was added in Python 3.10
I added a new |
Proposed change
This is a follow-up on #108.
In the current implementation, analysis settings can be specified using both a settings object and keyword arguments.
In some cases, this implementation can prevent changing modified settings back to their default values.
Implementations that don't exhibit this behavior rely on
**kwargs
, which means we lose the specification of the default arguments in the function signature.The
__init__
method of analysis wrappers now only allows to specify the settings using keyword arguments. Initializing an analysis with a settings object can be done using thefrom_settings
classmethod, e.g.ZernikeStandardCoefficients.from_settings(ZernikeStandardCoefficientsSettings(sampling="64x64", maximum_term=45))
. A downside of this implementation is that we lose the ability to create a new 'default' set of settings and then easily overwrite some of these settings, which was a major reason to choose the previously proposed implementation, but we can probably live with that.Other (related) changes:
class PolarizationTransmission( BaseAnalysisWrapper[PolarizationTransmissionResult, PolarizationTransmissionSettings]): ...
is sufficient to letBaseAnalysisWrapper
find out that settings should have thePolarizationTransmissionSettings
type. As a result, it is no longer necessary to create aPolarizationTransmissionSettings
instance inPolarizationTransmission.__init__
.Type of change
Additional information
Related issues
Checklist
If you contributed an analysis:
AttrDict
s for the analysis result data (please use dataclasses instead).If you contributed an example: