openmc icon indicating copy to clipboard operation
openmc copied to clipboard

Feature: Add model.description

Open ondrejch opened this issue 3 months ago • 1 comments

Description

This pull request introduces a description attribute to the Model class, allowing users to add a descriptive text to their models. This description is saved in the settings.xml output.

Key changes:

  • Added description property to openmc.model.Model.
  • Updated openmc.settings.Settings to include and handle the description in XML exports.
  • Added documentation for the new feature.
  • Included unit tests to verify the new functionality.

Fixes # 3586

Checklist

  • [x] I have performed a self-review of my own code
  • [N/A] I have run clang-format (version 15) on any C++ source files (if applicable)
  • [x] I have followed the style guidelines for Python source files (if applicable)
  • [x] I have made corresponding changes to the documentation (if applicable)
  • [x] I have added tests that prove my fix is effective or that my feature works (if applicable)

ondrejch avatar Oct 03 '25 23:10 ondrejch

This is a good start, but I have a few concerns about this PR first. Overall I think I do like the idea of this feature. This is much more like MCNP with problem titles, though this should never be mandatory. If a user keeps this up to date it could be helpful for figuring out what file is which.

  1. This doesn't feel idiomatic to OpenMC. I think Model.name would be more idiomatic to keep with Cell.name.
  2. This is changing the XML schema.
    1. Does this require an update to the schema specifications?
    2. Should this be universal for all XML files, and not just model.xml?
    3. Is testing on the C++ side necessary to ensure that the C++ doesn't crash when encountering an extra node?
  3. Should this be parroted out during transport?
  4. A separate test file I think will lead to confusion, these tests should probably be incorporated elsewhere.
  5. I don't see the utility in openmc.Settings.description. IMHO a description/name should be a property of the Model only, and not settings.

MicahGale avatar Oct 28 '25 20:10 MicahGale