Skip to content

Unified Aero Recipe Tidying#1753

Merged
peterdsharpe merged 9 commits into
NVIDIA:mainfrom
peterdsharpe:psharpe/unified-recipe-minor-cleanups
Jun 25, 2026
Merged

Unified Aero Recipe Tidying#1753
peterdsharpe merged 9 commits into
NVIDIA:mainfrom
peterdsharpe:psharpe/unified-recipe-minor-cleanups

Conversation

@peterdsharpe

Copy link
Copy Markdown
Collaborator

PhysicsNeMo Pull Request

Description

This PR does some cleanup on the Unified External Aero Recipe. Specifically:

  • In README.md, deduplicates some sections, removes the stale warning header, and links in DoMINO.
  • In metrics.py:
    • Removes the unused process_group in MetricCalculator.
    • Removes unused MetricCalculator.total_channels
    • Removes the unused field_dim import
  • In train.py:
    • Removes a duplicated omegaconf import
  • In nondim.py:
    • Removes the unused inverse_tensor(), which is both a) not used, and b) already superseded by the already-existing inverse() and inverse_td().

Checklist

Dependencies

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

@copy-pr-bot

copy-pr-bot Bot commented Jun 25, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@peterdsharpe

Copy link
Copy Markdown
Collaborator Author

/ok to test c103fd8

@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR tidies the Unified External Aero Recipe by removing dead code (inverse_tensor, _FIELD_CHANNELS, total_channels, process_group parameter, field_dim import, a duplicate omegaconf import) and condensing the README.

  • nondim.py: inverse_tensor() and its _FIELD_CHANNELS helper are removed; no callsites exist, so this is safe.
  • train.py: The redundant import omegaconf is dropped and both usages are correctly updated to the already-imported OmegaConf name.
  • metrics.py: The process_group parameter is removed from MetricCalculator.__init__, but _all_reduce still reads self.process_group — causing an AttributeError on every metric call.

Important Files Changed

Filename Overview
examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/metrics.py Removes process_group parameter and total_channels from MetricCalculator, but _all_reduce still references self.process_group, causing an AttributeError on every metric call.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/nondim.py Removes unused inverse_tensor() method and its supporting _FIELD_CHANNELS dict; no other callsites exist in the recipe.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/train.py Removes redundant import omegaconf module-level import; both call sites updated to use the already-imported OmegaConf name correctly.
examples/cfd/external_aerodynamics/unified_external_aero_recipe/README.md Documentation-only cleanup: removes stale warning header, deduplicates sections, and adds DoMINO to the supported-models list.

Comments Outside Diff (1)

  1. examples/cfd/external_aerodynamics/unified_external_aero_recipe/src/metrics.py, line 221-233 (link)

    P0 self.process_group removed from __init__ but still referenced in _all_reduce

    The PR removes self.process_group = process_group from __init__, but _all_reduce still reads self.process_group at lines 222, 224, and 233. Since every __call__ unconditionally passes its result through _all_reduce (line 287), any metric computation will immediately raise AttributeError: 'MetricCalculator' object has no attribute 'process_group'. Either the _all_reduce body must be updated to hard-code None/single-group behaviour, or self.process_group = None needs to be restored to __init__.

Reviews (1): Last reviewed commit: "Merge branch 'main' into psharpe/unified..." | Re-trigger Greptile

@peterdsharpe

Copy link
Copy Markdown
Collaborator Author

/ok to test a4e0d46

@peterdsharpe peterdsharpe enabled auto-merge June 25, 2026 15:24
@peterdsharpe peterdsharpe added this pull request to the merge queue Jun 25, 2026
Merged via the queue into NVIDIA:main with commit 8f0f098 Jun 25, 2026
6 checks passed
@peterdsharpe peterdsharpe deleted the psharpe/unified-recipe-minor-cleanups branch June 25, 2026 16:04
coreyjadams pushed a commit that referenced this pull request Jun 26, 2026
* deduplication

* cuts the warning

* Metrics: removes total_channels and field_dim

* train.py - omegaconf tidying

* In nondim, removes `inverse_tensor`, which is already superseded by either `inverse()` or `inverse_td()`. And never used

* grammar

* Adds caveat on DoMINO
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