Skip to content

Conversation

@GiovanniCanali
Copy link
Collaborator

@GiovanniCanali GiovanniCanali commented Nov 11, 2025

Description

This PR fixes #574

Checklist

  • Code follows the project’s Code Style Guidelines
  • Tests have been added or updated
  • Documentation has been updated if necessary
  • Pull request is linked to an open issue

@GiovanniCanali GiovanniCanali self-assigned this Nov 11, 2025
@GiovanniCanali GiovanniCanali force-pushed the domain branch 2 times, most recently from 03ab912 to f03bfa9 Compare November 11, 2025 17:05
@GiovanniCanali GiovanniCanali added enhancement New feature or request pr-to-fix Label for PR that needs modification low priority Low priority fix labels Nov 11, 2025
@GiovanniCanali GiovanniCanali force-pushed the domain branch 14 times, most recently from b784158 to 69510e6 Compare November 13, 2025 16:10
@GiovanniCanali
Copy link
Collaborator Author

This PR introduces the following improvements:

  • Renames files to align with the PINA naming conventions (see Names convention #445).
  • Separates the abstract interface from the base classes (see Adding base classes with only method definition #672).
  • Implements the partial method for all domains and set operations, enabling full-boundary retrieval.
  • Fixes uniform sampling for EllipsoidDomain and SimplexDomain, ensuring a truly uniform distribution.
  • Redefines the sampling strategy for improved speed and robustness.
  • Adds stricter, shared validation checks across all domains to prevent initialization issues.
  • Expands the test suite for domains and set operations.
  • Updates tutorial6.

These modifications do not affect the user experience, aside from the new additions.

@GiovanniCanali GiovanniCanali force-pushed the domain branch 2 times, most recently from f62602f to 0395c89 Compare November 19, 2025 13:58
@GiovanniCanali GiovanniCanali added pr-to-review Label for PR that are ready to been reviewed and removed pr-to-fix Label for PR that needs modification labels Nov 19, 2025
@GiovanniCanali GiovanniCanali marked this pull request as ready for review November 19, 2025 14:06
@GiovanniCanali GiovanniCanali requested review from a team and dario-coscia as code owners November 19, 2025 14:06
@GiovanniCanali
Copy link
Collaborator Author

MEMO: if #717 is merged into dev before this PR, fix the conditions related to the spatial boundaries (use partial method).

@GiovanniCanali GiovanniCanali force-pushed the domain branch 3 times, most recently from ef8e89a to d560461 Compare November 25, 2025 08:28
Copy link
Member

@FilippoOlivo FilippoOlivo left a 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

pts = pts.as_subclass(LabelTensor)
pts.labels = labels

return pts[sorted(pts.labels)]
Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

@GiovanniCanali GiovanniCanali Dec 10, 2025

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.

Copy link
Collaborator

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

@GiovanniCanali GiovanniCanali force-pushed the domain branch 3 times, most recently from 5503a46 to 8b0b4aa Compare December 4, 2025 16:07
@GiovanniCanali GiovanniCanali added pr-to-fix Label for PR that needs modification and removed pr-to-review Label for PR that are ready to been reviewed labels Dec 10, 2025
@GiovanniCanali GiovanniCanali added pr-to-review Label for PR that are ready to been reviewed and removed pr-to-fix Label for PR that needs modification labels Dec 10, 2025
Copy link
Collaborator

@dario-coscia dario-coscia left a 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

@FilippoOlivo FilippoOlivo merged commit e69cecc into mathLab:dev Dec 11, 2025
17 of 19 checks passed
@GiovanniCanali GiovanniCanali deleted the domain branch December 11, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request low priority Low priority fix pr-to-review Label for PR that are ready to been reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants