Skip to content

Type tweak: Allow ZarrMonitor to take in Comm | DummyComm | None#290

Closed
jjuyeonkim wants to merge 2 commits into
NOAA-GFDL:developfrom
jjuyeonkim:20251028_type_tweak
Closed

Type tweak: Allow ZarrMonitor to take in Comm | DummyComm | None#290
jjuyeonkim wants to merge 2 commits into
NOAA-GFDL:developfrom
jjuyeonkim:20251028_type_tweak

Conversation

@jjuyeonkim
Copy link
Copy Markdown
Collaborator

@jjuyeonkim jjuyeonkim commented Oct 28, 2025

Description

Small proposed type adjustment for the Zarr Monitor. Without this, there are type linting errors when attempting to update the submodules for Pace: NOAA-GFDL/pace#156

How has this been tested?

CI passes, linting passes in Pace locally with this update

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (e.g. add new modules to docs/docstrings/)
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included

@jjuyeonkim jjuyeonkim requested a review from fmalatino October 28, 2025 16:03
@jjuyeonkim jjuyeonkim marked this pull request as ready for review October 28, 2025 16:46
@jjuyeonkim jjuyeonkim requested a review from romanc October 28, 2025 17:13
partitioner: Partitioner,
mode: str = "w",
mpi_comm: DummyComm | None = None,
mpi_comm: Comm | DummyComm | None = None,
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think an alternative could be to try having mpi_comm: Comm | None = None. Then, maybe have DummyComm inherit from Comm.

Copy link
Copy Markdown
Collaborator

@romanc romanc left a comment

Choose a reason for hiding this comment

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

yeah, ZarrMonitor should allow any Comm object and DummyComm should inherit from Comm. I made an alternative PR in #292.

@jjuyeonkim jjuyeonkim closed this Oct 29, 2025
@jjuyeonkim jjuyeonkim deleted the 20251028_type_tweak branch October 29, 2025 14:57
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.

3 participants