Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Export large weight window files through openmc.lib without XML serialization by paulromano · Pull Request #4057 · openmc-dev/openmc · GitHub
Skip to content

Export large weight window files through openmc.lib without XML serialization - #4057

Open
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export
Open

Export large weight window files through openmc.lib without XML serialization#4057
paulromano wants to merge 8 commits into
openmc-dev:developfrom
paulromano:wwinp-direct-lib-export

Conversation

@paulromano

Copy link
Copy Markdown
Contributor

Description

Background

WeightWindowsList.export_to_hdf5() currently creates a temporary model containing the weight windows, writes that model to XML, initializes the OpenMC shared library from the XML, and then calls the existing C++ HDF5 exporter. For weight window files containing hundreds of millions of values, constructing the ASCII representation of the bounds can require multiple GBs of additional memory and eventually raise MemoryError.

#3942 addressed this by implementing a direct HDF5 writer in Python with h5py. #3951 refined that design by moving mesh serialization into methods on each mesh subclass. Both approaches avoid the large XML document, but they introduce a second implementation of the weight window HDF5 format alongside the existing C++ writer. This duplicates format logic across Python and C++, requires the Python implementation to remain synchronized with future format changes, and makes the C API responsible for passing HDF5-specific hid_t values across the language boundary. PR #3951 also introduces a separate cleanup operation to manage objects created for this export path.

Approach

This PR avoids XML serialization while continuing to use the existing C++ HDF5 writer. WeightWindowsList.export_to_hdf5() initializes a minimal temporary OpenMC library session and creates the required meshes and weight windows directly through openmc.lib. Once the C++ objects have been populated, it calls the existing openmc.lib.export_weight_windows() function. The weight window bounds therefore never need to be represented in XML.

A new public MeshBase.to_lib_object() method creates the corresponding runtime mesh in an initialized OpenMC library session. It supports regular, rectilinear, cylindrical, spherical, and unstructured meshes. The necessary mesh names, origins, unstructured-mesh options, length multipliers, and IDs are transferred through APIs using ordinary C-compatible data types. The existing openmc_add_unstructured_mesh() API is extended to accept all properties needed to reproduce a Python unstructured mesh directly in the library.

This design retains the primary benefit of PRs #3942 and #3951: multi-GB weight window data no longer passes through XML. Unlike those approaches, it preserves a single implementation of the weight window HDF5 format. Changes to the format only need to be made in the existing C++ writer, and Python does not need to reproduce C++ serialization behavior with h5py.

Checklist

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

Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IMO, each mesh subclass should implement how to convert itself to its lib counterpart.
That way we don't have to edit this function when implementing a new mesh type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Agreed. I moved the conversion to openmc.lib as openmc.lib.Mesh.from_python(mesh). Each concrete openmc.lib mesh subclass now implements its own _from_python() conversion hook, while the base method only selects the matching subclass and handles shared behavior such as the initialization check, ID, name, and base directory.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks nice @paulromano! Good idea to address the duplicate implementation issue. There's more than enough to keep track of as it is.

Some design conversation to he bad about which side (Python API or openmc.lib) should initiate the object transfers.

Comment threadopenmc/weight_windows.py Outdated
lib_meshes[mesh.id] = mesh.to_lib_object(
base_dir=original_dir)

lib_ww = openmc.lib.WeightWindows(ww.id)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the openmc.lib.WeightWindows class could have a classmethod that takes in an openmc.WeightWindows object to handle some of the setup going on here.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good suggestion! I've added openmc.lib.WeightWindows.from_python(), which limits the logic here to managing the temporary session, deduplicating shared meshes, invoking the conversion, and calling the existing C++ exporter.

Comment threadinclude/openmc/capi.h
int openmc_mesh_filter_set_translation(int32_t index, double translation[3]);
int openmc_mesh_get_id(int32_t index, int32_t* id);
int openmc_mesh_set_id(int32_t index, int32_t id);
int openmc_mesh_get_name(int32_t index, const char** name);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's add some tests for these new functions in test_lib.py

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Implemented new tests that cover these

Comment threadtests/unit_tests/test_checkvalue.py
Comment threadopenmc/mesh.py Outdated
uid = self.id
base_dir = Path.cwd() if base_dir is None else Path(base_dir)

if isinstance(self, RegularMesh):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree.

In the vein of my other comment, this also feels appropriate for a classmethod approach on the openmc.lib.Mesh object where each subclass handles its own property settings for the resulting openmc.lib object.

e.g.

lib_mesh=openmc.lib.Mesh.from_python_object(spherical_mesh).

This makes it more natural (to me) to ensure that the openmc.lib module is initialized before performing these operations.

@paulromano

Copy link
Copy Markdown
ContributorAuthor

Thanks @GuySten and @pshriwise for the review! All your comments have been addressed.

@pshriwisepshriwise left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once last small comment from me, but otherwise I think this looks great!

Comment threadopenmc/lib/mesh.py
----------
mesh : openmc.MeshBase
Python API mesh to convert.
uid : int, optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this parameter used anywhere in the code currently?

@GuySten

Copy link
Copy Markdown
Contributor

I suggest waiting for #4091. Which touches the same code.

Sign up for freeto 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

@paulromano@GuySten@pshriwise