-
Notifications
You must be signed in to change notification settings - Fork 0
Improved Aliases & Constraints + Enhanced Parameter Handling #28
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
Conversation
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.
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/easydiffraction/analysis/analysis.py:183
- Consider using the 'uid' property of the parameter instead of directly calling _generate_human_readable_unique_id(), to ensure consistency and avoid unnecessary regeneration.
uid = param._generate_human_readable_unique_id()
src/easydiffraction/analysis/analysis.py:414
- [nitpick] Ensure that 'current_minimizer' is not already quoted before adding quotes to avoid potential formatting issues in the exported CIF.
current_minimizer = self.current_minimizer
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 don't think there is anything wrong here, but the amount of code is large, so I might've missed a thing or two.
|
||
# Lock further attribute additions to prevent | ||
# accidental modifications by users | ||
self._locked = True |
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.
The whole aspect of _locked
attributes is well conceived. However, there are cases where we would like to extend the list of attributes (e.g. in a derived class). Maybe we can add a method on the Component class, which would allow this? Say add_attribute
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.
Well, right now we have the _locked
attribute in the Component
class, which is False
by default. In derived classes like Cell
, I just create the required attributes in __init__
, and then set _locked = True
at the end.
Are you thinking about a third class that inherits from Cell
? If so, in that case you can just set _locked = False
again in the __init__
of the subclass, add whatever extra attributes you need, and then re-lock it with _locked = True
, right?
Or are you suggesting that having a dedicated add_attribute
method would be a cleaner or safer way to manage this? Or maybe you're thinking about allowing users to attach custom attributes directly? Just trying to understand which use case you're targeting.
Also, I’m not happy with this implementation. We have lots of classes inheriting from Component
, and I’ve already run into cases where I simply forgot to set _locked = True
at the end of __init__
. It would be nice to find a more advanced way to achieve this functionality.
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'm trying to allow for cases where we use locked
classes as base classes, e.g. ConstantWavelengthInstrument
as the base class used with mix-in PDF base class.
_locked
prohibits me from adding new, PDF specific parameters to the ConstantWavelengthInstrument
-derived class, but if we had a separate add_attribute
method, this becomes straightforward.
This is what I implemented in the PDF branch, btw.
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.
Ah, I see. The current design was originally based on the assumption that instrument classes wouldn’t use mixins. We only had two types: ConstantWavelengthInstrument
and TimeOfFlightInstrument
, both inheriting from InstrumentBase
. Importantly, InstrumentBase
doesn’t lock attribute creation, but the two concrete instrument classes do by setting _locked = True
at the end of __init__
.
Since PDFCWInstrument
inherits from ConstantWavelengthInstrument
, it is locked by the base class, and we can’t just reset it to False because Component.__setattr__
prevents that once locked.
So we have several options:
- Add a separate
add_attribute
method as you suggested. - Restructure the hierarchy, similar to what we do for
Peak
, and havePDFCWInstrument
inherit directly fromInstrumentBase
rather than fromConstantWavelengthInstrument
. - Redesign the entire
Instrument
class structure to better support this kind of composition.
I’m currently rethinking the broader structure of Instrument
and related Experiment
components to see how they can better accommodate PDF analysis.
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.
My naive thinking is that we should allow for flexibility in those derived classes, as we might want to potentially use them as base classes for other cases, like single crystal, 2D, or else.
I think that the current design is well thought out and should work well when extended to other cases.
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 thought this was something we specifically wanted to avoid. My original design proposal (in https://github.com/orgs/easyscience/discussions/29) followed a multi-level inheritance structure like BaseMeasurement
-> Measurement_Pd
-> Measurement_PdCwl
, etc. But after our discussions, we agreed to move away from that and instead construct experiment classes from smaller, reusable components.
That’s when I introduced the approach of having a single base class and mixing in additional components as needed. An example is how things are structured in peak.py, where each class derived from PeakBase
is considered final and we don’t subclass further. Instead, mixins are used to compose the necessary behavior.
I think this approach is simpler, cleaner, and easier to maintain long-term. So I’d prefer we stick with this idea: avoid inheritance chains and favor composition via mixins.
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.
Feel free to merge
This PR brings major improvements to how we handle aliases, constraints, and parameters in general. You can now export aliases and constraints directly to CIF format. There’s also a cleanup and refactor of parameter handling, base classes, and unique ID generation - now using more human-readable formats.
To help users interactively explore parameters in code, the following helper is available:
Example output:
You can define aliases and constraints like this:
And here’s how they appear in the analysis.cif: