Skip to content

refactor: prefix VelocityAdvection internal attributes with underscore#1180

Merged
jcanton merged 5 commits intomainfrom
copilot/make-self-names-consistent
Apr 16, 2026
Merged

refactor: prefix VelocityAdvection internal attributes with underscore#1180
jcanton merged 5 commits intomainfrom
copilot/make-self-names-consistent

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 30, 2026 •
edited
Loading

  • Rename self.grid → self._grid in VelocityAdvection
  • Rename self.metric_state → self._metric_state in VelocityAdvection
  • Rename self.interpolation_state → self._interpolation_state in VelocityAdvection
  • Rename self.vertical_params → self._vertical_params in VelocityAdvection
  • Rename self.edge_params → self._edge_params in VelocityAdvection
  • Rename self.c_owner_mask → self._c_owner_mask in VelocityAdvection
  • Rename self.cfl_w_limit → self._cfl_w_limit in VelocityAdvection
  • Rename self.scalfac_exdiff → self._scalfac_exdiff in VelocityAdvection
  • Rename self.vertical_cfl → self._vertical_cfl in VelocityAdvection
  • Add type hints to _vertical_params, _edge_params, _c_owner_mask
  • Update test references to use _ prefix
  • Updated branch to latest origin/main

⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.

@jenkins-apn
Copy link
Copy Markdown

jenkins-apn commented Mar 30, 2026

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

1 similar comment
@jenkins-apn
Copy link
Copy Markdown

jenkins-apn commented Mar 30, 2026

Hi there, this is jenkins continuous integration...
Do you want me to verify this patch?

Copilot AI linked an issue Mar 30, 2026 that may be closed by this pull request
Make self. names consistent. E.g. VelocityAdvection has self.grid instead of self._grid #1156
Closed
@jcanton
refactor: make VelocityAdvection instance attribute names consistent …
a93b8b3 
…with underscore convention

Rename internal state attributes in VelocityAdvection to use underscore prefix,
matching the convention used in SolveNonhydro and Diffusion:
- self.grid -> self._grid
- self.metric_state -> self._metric_state
- self.interpolation_state -> self._interpolation_state
- self.vertical_params -> self._vertical_params
- self.edge_params -> self._edge_params
- self.c_owner_mask -> self._c_owner_mask

Public attributes (cfl_w_limit, scalfac_exdiff, vertical_cfl) remain unchanged
as they are part of the external API.

Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/551a6408-1d07-45e9-a16d-b0c9974aed25

Co-authored-by: jcanton <5622559+jcanton@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix inconsistent self. variable names refactor: prefix VelocityAdvection internal attributes with underscore Mar 30, 2026
Copilot AI requested a review from jcanton March 30, 2026 09:43
@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Mar 30, 2026

cscs-ci run default

@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Mar 30, 2026

cscs-ci run distributed

msimberg
msimberg approved these changes Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@msimberg msimberg left a comment

Choose a reason for hiding this comment

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

This looks fine and self contained to me assuming tests pass.

@msimberg
Copy link
Copy Markdown
Contributor

msimberg commented Apr 15, 2026

@copilot update the branch to latest origin/main.

Merge remote-tracking branch 'origin/main' into copilot/make-self-nam…
625b02f 
…es-consistent

Co-authored-by: msimberg <42977+msimberg@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 15, 2026

@copilot update the branch to latest origin/main.

Done — merged latest origin/main into the branch at 625b02f5. No conflicts.

Copilot AI requested a review from msimberg April 15, 2026 11:00
@msimberg msimberg requested review from OngChia and mattmunnich April 15, 2026 11:10
@msimberg msimberg marked this pull request as ready for review April 15, 2026 11:10
@OngChia
Copy link
Copy Markdown
Contributor

OngChia commented Apr 15, 2026 •
edited
Loading

I would say these three variables self.cfl_w_limit, self.scalfac_exdiff, self.vertical_cfl are also local variables. They appear only in data test against the serialized data. They are all intermediate variables to compute global variables.

@OngChia OngChia self-requested a review April 15, 2026 11:35
OngChia
OngChia reviewed Apr 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@OngChia OngChia left a comment

Choose a reason for hiding this comment

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

#1180 (comment)

@msimberg msimberg requested review from nfarabullini and removed request for mattmunnich April 15, 2026 12:14
@msimberg
Copy link
Copy Markdown
Contributor

msimberg commented Apr 15, 2026

I would say these three variables self.cfl_w_limit, self.scalfac_exdiff, self.vertical_cfl are also local variables. They appear only in data test against the serialized data. They are all intermediate variables to compute global variables.

@copilot can you take care of this comment? If the only place where they're accessed "externally" is in tests, just access them with _x names there.

@msimberg
refactor: make cfl_w_limit, scalfac_exdiff, vertical_cfl private in V…
5923d0b 
…elocityAdvection

Agent-Logs-Url: https://github.com/C2SM/icon4py/sessions/f4210f8b-74a8-44d5-bece-81b5a6d75aab

Co-authored-by: msimberg <42977+msimberg@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 15, 2026

I would say these three variables self.cfl_w_limit, self.scalfac_exdiff, self.vertical_cfl are also local variables. They appear only in data test against the serialized data. They are all int...

Done in 5923d0b — renamed self.cfl_w_limit → self._cfl_w_limit, self.scalfac_exdiff → self._scalfac_exdiff, self.vertical_cfl → self._vertical_cfl in velocity_advection.py, and updated the three test assertions in test_velocity_advection.py to use the _ prefixed names.

nfarabullini
nfarabullini reviewed Apr 15, 2026
View reviewed changes
Comment thread model/atmosphere/dycore/src/icon4py/model/atmosphere/dycore/velocity_advection.py Outdated
Comment on lines +59 to +61
self._vertical_params = vertical_params
self._edge_params = edge_params
self._c_owner_mask = owner_mask
Copy link
Copy Markdown
Contributor

@nfarabullini nfarabullini Apr 15, 2026

Choose a reason for hiding this comment

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

should these three also have type hints?

Copy link
Copy Markdown
Contributor

@msimberg msimberg Apr 15, 2026

Choose a reason for hiding this comment

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

Would be good. @copilot can you add type hints?

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

Added type hints in e02c9ff: _vertical_params: v_grid.VerticalGrid, _edge_params: grid_states.EdgeParams, _c_owner_mask: fa.CellField[bool] — now all nine attributes in __init__ have explicit type annotations.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 15, 2026

Mandatory Tests

Please make sure you run these tests via comment before you merge!

  • cscs-ci run default
  • cscs-ci run distributed

Optional Tests

To run benchmarks you can use:

  • cscs-ci run benchmark-bencher

To run tests and benchmarks with the DaCe backend you can use:

  • cscs-ci run dace

To run test levels ignored by the default test suite (mostly simple datatest for static fields computations) you can use:

  • cscs-ci run extra

For more detailed information please look at CI in the EXCLAIM universe.

@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Apr 15, 2026

cscs-ci run default

@jcanton
Copy link
Copy Markdown
Contributor

jcanton commented Apr 15, 2026

cscs-ci run distributed

@jcanton jcanton merged commit 130f04a into main Apr 16, 2026
54 checks passed
jcanton added a commit that referenced this pull request Apr 16, 2026
@jcanton
Merge branch 'main' into distributed_standalone_driver
adec339 
* main:
  refactor: prefix VelocityAdvection internal attributes with underscore (#1180)
  fortran bindings: split py2fgen and bindings (#1185)
  Rename `single_node_default` to `single_node_exchange` (#1176)
  remove the duplication of fixtures (#1186)
  Update GT4Py to v1.1.9 (#1187)
  cleanup 3 unused vars from solve_nh_init (#1169)
@msimberg msimberg deleted the copilot/make-self-names-consistent branch April 17, 2026 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@OngChia OngChia OngChia approved these changes

@nfarabullini nfarabullini nfarabullini approved these changes

@jcanton jcanton Awaiting requested review from jcanton

@msimberg msimberg Awaiting requested review from msimberg

Assignees

@jcanton jcanton

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make self. names consistent. E.g. VelocityAdvection has self.grid instead of self._grid

6 participants

@jenkins-apn @jcanton @msimberg @OngChia @nfarabullini