-
Notifications
You must be signed in to change notification settings - Fork 91
Domain module refactoring #699
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
base: dev
Are you sure you want to change the base?
Conversation
03ab912 to
f03bfa9
Compare
b784158 to
69510e6
Compare
|
This PR introduces the following improvements:
These modifications do not affect the user experience, aside from the new additions. |
16c6c8f to
736639a
Compare
f62602f to
0395c89
Compare
0395c89 to
b18d184
Compare
|
MEMO: if #717 is merged into |
b18d184 to
3adf926
Compare
3adf926 to
ef8e89a
Compare
ef8e89a to
d560461
Compare
FilippoOlivo
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.
Hi @GiovanniCanali, thank you for the PR. It is a great improvement PINA geometries. I started the review and I have some doubts about how BaseDomain is structured. Specifically in how sampling modes are handled. I leaved some comments on the involved lines of code. I will continue the review and give you a feedback also on other classes soon
| """ | ||
|
|
||
| # Define available sampling modes | ||
| __AVAILABLE_MODES = ("random", "grid", "lh", "chebyshev", "latin") |
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.
Are these available for all the domains?
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.
__AVAILABLE_MODES stores all sampling modes defined in PINA. Each domain generally uses only a subset of these modes. In the __init__ of every class inheriting from BaseDomain, you’ll find a line like:
self.sample_modes = [...]This calls the corresponding setter, which serves two purposes:
- It defines the sampling modes available for that specific domain.
- It validates the mode defined in the
__init__, raising an error if it is not included in__AVAILABLE_MODES.
The only exception is CartesianDomain, which does not call the setter (it does not even have a __init__), because its available modes coincide exactly with the full set in __AVAILABLE_MODES.
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.
Basically, this is just a variable storing all available modes. Through the setter, each domain defines a subset of these values.
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.
In my opinion, it is better to have a distance __AVAILABLE_MODES for each subclass
| # Define available sampling modes | ||
| __AVAILABLE_MODES = ("random", "grid", "lh", "chebyshev", "latin") | ||
|
|
||
| def __init__(self, domain_dict=None): |
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 the name of the variable is not the best one. Can you rename it like "variables_dict"?
| for v in domain_dict.values(): | ||
| check_consistency(v, (int, float)) | ||
|
|
||
| # Iterate over domain_dict items |
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.
Can you make a separate function for that?
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.
Deleted previous comment, that was my mistake.
I would keep this logic inside the same function: the base class __init__ performs all necessary checks, and this behavior belongs to that same scope.
Description
This PR fixes #574
Checklist