Skip to content

Conversation

@gthyagi
Copy link
Contributor

@gthyagi gthyagi commented Jun 26, 2025

The checkpoint_xdmf method has been updated to correctly save discontinuous fields.

@gthyagi gthyagi requested review from Copilot and julesghub and removed request for Copilot June 26, 2025 06:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the checkpoint_xdmf method to distinguish between discontinuous (cell-based) and continuous (node-based) fields, ensuring correct file naming and attribute generation in the XDMF output. Key changes include:

  • Adjusting the filename format for mesh-variable HDF5 files.
  • Adding a helper function and conditional logic to select between cell_fields and vertex_fields based on the variable’s continuity.
  • Adding a comprehensive test to verify that discontinuous fields are correctly saved and referenced.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/test_0005_check_xdmf.py Introduces tests to check that XDMF files correctly reference HDF5 datasets for mesh fields.
src/underworld3/discretisation.py Modifies checkpoint_xdmf to support discontinuous fields and updates file naming and data paths.

var_filename = filename + f".mesh.{var.clean_name}.{index:05}.h5"

def get_cell_field_shape(h5_filename):
with h5py.File(h5_filename, 'r') as f:
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

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

The helper function 'get_cell_field_shape' assumes that the expected dataset exists in the HDF5 file. Consider adding error handling to provide a clearer message if the dataset is missing.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

If not cell fields are found what does this return? Is it well handled?

Copy link
Member

@julesghub julesghub left a comment

Choose a reason for hiding this comment

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

Looks good - just see how it handles if no cell_fields are defined.

var_filename = filename + f".mesh.{var.clean_name}.{index:05}.h5"

def get_cell_field_shape(h5_filename):
with h5py.File(h5_filename, 'r') as f:
Copy link
Member

Choose a reason for hiding this comment

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

If not cell fields are found what does this return? Is it well handled?

@gthyagi
Copy link
Contributor Author

gthyagi commented Jun 26, 2025

Yes, it is handled properly. not getattr(var, "continuous") or getattr(var, "degree")==0 this will ensure get_cell_field_shape called only for discontinuous or degree=0 mesh variable.

# Determine if data is stored on nodes (vertex_fields) or cells (cell_fields)
if not getattr(var, "continuous") or getattr(var, "degree")==0:
    center = "Cell"
    numItems = get_cell_field_shape(var_filename)
    field_group = "cell_fields"
else:
    center = "Node"
    numItems = numVertices
    field_group = "vertex_fields"

var_filename = filename + f"mesh.{var.clean_name}.{index:05}.h5"
var_filename = filename + f".mesh.{var.clean_name}.{index:05}.h5"

def get_cell_field_shape(h5_filename):
Copy link
Member

Choose a reason for hiding this comment

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

on second inspection I don't think this function should be called get..._shape. Consider get...._size.

Also can you not define this function in the for var in meshVars loop. This is not a good design. Move the function out of the loop. That will make it much less coupled to the loop.

@gthyagi
Copy link
Contributor Author

gthyagi commented Jun 27, 2025

check #328

@gthyagi gthyagi closed this Jun 27, 2025
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