Skip to content

Conversation

@RiccardoRossi
Copy link
Member

📝 Description
this is a tensor adaptor which collects the node ids from geometries.

🆕 Changelog

@rubenzorrilla
Copy link
Member

I'd suggest naming it GeometryIdTensorAdaptor. I think IndexTensorAdaptor is not very self-descriptive.

Copy link
Contributor

@matekelemen matekelemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, after looking at the tests and reading the PR's title, I understand what yoou're doing, but the class' name is unbelievably misleading.

To me, it was clear as day that GeometryIdsTensorAdaptor fetches the IDs of geometries. But no, it fetches the node IDs of each geometry. Please rename it to something more intuitive.

@RiccardoRossi
Copy link
Member Author

Ok, after looking at the tests and reading the PR's title, I understand what yoou're doing, but the class' name is unbelievably misleading.

To me, it was clear as day that GeometryIdsTensorAdaptor fetches the IDs of geometries. But no, it fetches the node IDs of each geometry. Please rename it to something more intuitive.

ConnectivityIdsTrnsorAdaptor?

I don t mind changing the name but i am out of ideas...suggestions are welcomd

On my side,
I am worried about

  1. what the copy constructor should do
  2. should we allow changing the ids? I woukd say no, otherwise we need to also pass the modelpart nodes in the constructot

Copy link
Member

@sunethwarna sunethwarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am ok with this, with the minor concerns...

@RiccardoRossi
Copy link
Member Author

@sunethwarna in principle it is ready (with your suggestions and a renamed name)

Copy link
Member

@sunethwarna sunethwarna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RiccardoRossi :)

@RiccardoRossi RiccardoRossi merged commit 8c81480 into master Jan 9, 2026
10 checks passed
@RiccardoRossi RiccardoRossi deleted the add_index_tensor_adaptor branch January 9, 2026 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants