Skip to content
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

Domino multiscale geometry features capability and inference script #788

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

RishikeshRanade
Copy link
Collaborator

@RishikeshRanade RishikeshRanade commented Feb 10, 2025

Modulus Pull Request

Description

This PR adds a feature in domino to encode geometry using multi-scale features. It also provides an inference script to evaluate domino on unknown STLs and flow speeds.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • The CHANGELOG.md is up to date with these changes.
  • An issue is linked to this pull request.

Dependencies

Copy link
Collaborator

@Alexey-Kamenev Alexey-Kamenev left a comment

Choose a reason for hiding this comment

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

Some nitpicks but overall LGTM!

@@ -50,7 +50,7 @@ variables:
# The following is for AWS DrivAer dataset.
UMeanTrim: vector
pMeanTrim: scalar
nutMeanTrim: scalar
# nutMeanTrim: scalar
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just checking: this is supposed to be commented out, right?

@@ -233,6 +233,15 @@ def __getitem__(self, idx):
volume_coordinates = volume_coordinates[ids_in_bbox]
volume_fields = volume_fields[ids_in_bbox]

# volume_fields[:, :3] = volume_fields[:, :3] / STREAM_VELOCITY
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want to keep it commented out or remove? If the former - maybe add a comment/TODO.


self.bq_warp = nn.ModuleList()
self.geo_processors = nn.ModuleList()
for j, p in enumerate(radii):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: can use simply for r in radii

model_parameters=geometry_rep.geo_processor,
)
self.geo_conv_out = nn.ModuleList()
for j, p in enumerate(radii):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here - j and p are not used here at all. Can do something like:

for _ in range(len(radii)):

or just fold this loop into the previous one.

)
self.radius = model_parameters.geometry_local.radii
self.bq_warp = nn.ModuleList()
for ct, j in enumerate(self.radius):
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. What is ct?
  2. j looks like an index, but it's not - it's actual value of a radius.
    Maybe refactor it for clarity?


surf_max = s_max + (s_max - s_min) / 2
surf_min = s_min - (s_max - s_min) / 2
surf_min[2] = s_min[2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does it do? Is it z coordinate?

surf_sdf_grid = torch.unsqueeze(surf_sdf_grid, 0)
max_min = [c_min, c_max]
surf_max_min = [surf_min, surf_max]
center_of_mass = center_of_mass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does not seem to be doing anything?

surf_max_min = [surf_min, surf_max]
center_of_mass = center_of_mass

return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I would try not return 8-element tuple here. Maybe use dict or named tuple or dataclass.

)


class dominoInference:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use PascalCase for class names.


domino = dominoInference(cfg, dist, False)
domino.initialize_model(
model_path="/lustre/rranade/domino_automotive_aero_nim/automotive-aerodynamics-nim/nims/domino-automotive-aero/factory/triton/model/1/checkpoints/DoMINO.0.618.pt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use config, do not hardcode.

@RishikeshRanade
Copy link
Collaborator Author

/blossom-ci

1 similar comment
@Alexey-Kamenev
Copy link
Collaborator

/blossom-ci

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants