-
Notifications
You must be signed in to change notification settings - Fork 92
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
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 |
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
d560461 to
0d1fa28
Compare
| pts = pts.as_subclass(LabelTensor) | ||
| pts.labels = labels | ||
|
|
||
| return pts[sorted(pts.labels)] |
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 we sure the labels we return are in the same order as the abstract problem input_variables? This is something very important, maybe we can add in the base class something that does so after the sample method
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 will check this thoroughly
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 verified that, given how PINA handles data, the ordering of coordinates does not matter: sampled points are always retrieved by label, not by positional slicing. As long as the labels are consistent (which I checked), the entire PINA pipeline behaves correctly regardless of the internal ordering.
If, however, you want to guarantee that the sampled points follow exactly the same ordering as the problem’s input_variables, we can adjust the following line in abstract_problem.py::collect_data():
samples = self.discretised_domains[condition.domain]to:
samples = self.discretised_domains[condition.domain][self.input_variables]This ensures that, as soon as points are sampled for a problem, they are immediately reordered to match the input_variables ordering.
Note that this adjustment must be made at the problem level, not the domain level, since domains can be sampled independently of any problem definition.
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.
@dario-coscia please let me know how you prefer to proceed: keep the current behavior, or add the label reordering directly in the collect_data method.
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.
Let's go with samples = self.discretised_domains[condition.domain][self.input_variables], then I am ok for merging
5503a46 to
8b0b4aa
Compare
8b0b4aa to
e06e8ba
Compare
e06e8ba to
e894012
Compare
dario-coscia
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.
Just add samples = self.discretised_domains[condition.domain][self.input_variables], then I am ok with merging
Description
This PR fixes #574
Checklist