Review Tutorial and Bug Fixes 2 #1497
Conversation
There was a problem hiding this comment.
Thanks @ahnaf-tahmid-chowdhury for digging in to this one - I imagine it was challenging given some of the updates since it was first developed.
| " lptag += (((tag[right] - tag[ent])/(r-c)) - ((tag[ent] - tag[left])/(c-l))) / ((r-l)/2)\n", | ||
| "\n", | ||
| " # Retrieve the tag values for the entities\n", | ||
| " tag_data_ent = m.mesh.tag_get_data(tag, ent)[0]\n", |
There was a problem hiding this comment.
Was the previous syntax failing? We should look into why that is, because it’s supposed to support that syntax for clarity/brevity. I think the mesh syntax was updated after these tutorials were written, so some change was certainly needed, but maybe we can restore something more concise?
There was a problem hiding this comment.
Cell In[1], line 47, in laplace(tag, idx, m, shape)
45 right, r = rpoint(idx, n, coords, shape, m)
46 c = coords[n]
---> 47 lptag += (((tag[right] - tag[ent]) / (r - c)) - ((tag[ent] - tag[left]) / (c - l))) / ((r - l) / 2)
48 return lptag
TypeError: 'pymoab.tag.Tag' object is not subscriptableI am using MOAB 5.4 and also checked 5.3. We can't use the subscript operator [] on a pymoab.tag.Tag object. The Core is updated.
There was a problem hiding this comment.
I think we need to rethink this whole file updated with the newer (updated by @pshriwise 4 years ago...) mesh infrastructure.
There was a problem hiding this comment.
it appears that this file was updated with a couple of small changes as the interface changed but was never tested. I think it may be best to remove and/or rewrite it from scratch.
In particular, I am a little lost on how to best get the coordinates correctly with the current interface. The last time this code worked, we got a single set of coordinates from a passing in a structured mesh hex (which has 8 vertices) - and I'm not sure how that was behaving.
There was a problem hiding this comment.
While considering rewriting from scratch, presents an opportunity to integrate new built-in methods, simplifying tasks like timesteps, rendering, and reactor management.
tutorial/08-diffusion.ipynb
Outdated
| "\n", | ||
| "### Function: isinrod(ent, rx, radius=0.4)\n", | ||
| "\n", | ||
| "This function determines whether a given entity (hexahedral cell) is inside a control rod based on its coordinates. It takes the following parameters:\n", |
There was a problem hiding this comment.
I don’t think there is any control rod in this model, only a single fuel pin. This method determines whether a point is inside the pin (“fuel rod”) or not.
There was a problem hiding this comment.
Yes. I also have a doubt about this. In the earlier version, this function was referred to as a control rod. create_reactor function defines a simple model of a nuclear reactor with a single fuel pin and no actual control rods. The terminology used in the earlier version might have caused confusion.
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
|
Hi @gonuke. Since a new version of ENSDF is scheduled to be published in September, along with a Python API, I believe we can merge this PR for now if you do not have any further suggestions. I may create another PR if necessary. |
| " for idx in product(*[range(xyz - 1) for xyz in shape]):\n", | ||
| " ent = m.structured_get_hex(*idx)\n", | ||
| " phi_next[ent] = (max(D[ent] * laplace(phi, idx, m, shape) + \n", | ||
| " (k[ent] - 1.0) * Sigma_a[ent] * phi[ent], 0.0) + S[ent])*dt*2.2e5 + phi[ent]\n", | ||
| " ents = m.mesh.getEntities(iBase.Type.region)\n", | ||
| " phi[ents] = phi_next[ents]" | ||
| " # Get the value of phi for the entity\n", | ||
| " phi_val = m.mesh.tag_get_data(phi, ent)[0]\n", | ||
| " # Get the value of D for the entity\n", | ||
| " D_val = m.mesh.tag_get_data(D, ent)[0]\n", | ||
| " # Get the value of k for the entity\n", | ||
| " k_val = m.mesh.tag_get_data(k, ent)[0]\n", | ||
| " # Get the value of Sigma_a for the entity\n", | ||
| " Sigma_a_val = m.mesh.tag_get_data(Sigma_a, ent)[0]\n", | ||
| " # Get the value of S for the entity\n", | ||
| " S_val = m.mesh.tag_get_data(S, ent)[0]\n", | ||
| "\n", | ||
| " # Perform calculations using the retrieved values\n", | ||
| " phi_next_val = (max(\n", | ||
| " D_val * laplace(phi, idx, m, shape) +\n", | ||
| " (k_val - 1.0) * Sigma_a_val * phi_val, 0.0\n", | ||
| " ) + S_val) * dt * v + phi_val\n", | ||
| "\n", | ||
| " # Store the calculated phi_next_val back to the phi_next tag for the entity\n", | ||
| " m.mesh.tag_set_data(phi_next, ent, phi_next_val)\n", |
There was a problem hiding this comment.
I think this could look more like this (assuming some changes to laplace as well):
for idx in m.iter_structured_idx():
phi_val = m.phi[idx]
D_val = m.D[idx]
k_val = m.k[idx]
Sigma_a_val = m.Sigma_a[idx]
S_val = m.S[idx]
m.phi_next[idx] = ( max( D_val * laplace(m.phi, idx, m, shape) + (k_val - 1.0) * Sigma_a_val * phi_val, 0 ) + S_val * dt * v + phi_val
There was a problem hiding this comment.
The key is to have access to an idx that is consistent with the internal ordering of the PyNE structured mesh and then refer to that.
I still need to consider how best to change laplace to match, but it will also be simpler
There was a problem hiding this comment.
It seems the Mesh object has no attribute phi in the built-in pymoab module that comes with MOAB 5.4.1. I'm wondering which version you are using in this case or if there's any other module you could suggest?
There was a problem hiding this comment.
m is not of type pymoab.Mesh. It is a pyne.mesh object. When tags are added, they also become added as attributes of the object. So I don’t expect m.mesh.phi to exist but I do expect m.phi to exist.
There was a problem hiding this comment.
I have checked mesh.py and couldn't find any phi function available. I did come across a Mesh class, which seems to be a PyMOAB core instance. Since PyMOAB is frequently updated, it's possible that the 'phi' function has been removed. Unfortunately, I'm not familiar with how the phi function worked since I haven't seen its code before. I also tried searching for it in the MOAB repository here, but I couldn't find anything related to it there.
There was a problem hiding this comment.
Ping @pshriwise
There was a problem hiding this comment.
I think you are still misunderstanding. There is no phi method or function defined in mesh.py. It's a little more complicated/clever than that. When a tag is added to the mesh with any name, it is also added as an attribute of that mesh. So if you have a mesh called my_mesh and you add a tag called my_tag, then it becomes possible to refer directly to my_mesh.my_tag[mesh_index] where mesh_index somehow refers to a set of mesh elements.
So, the comments I have made are based on the fact that a tag named phi is being added to a PyNE mesh object. I don't think there should be no need to ever refer to the underlying pymoab mesh. The PyNE mesh interface should provide enough capability to do everythng.
Therefore, you will never find phi defined anywhere in the PyNE code. This tutorial was originally written with a similar but subtly different API for mesh. I think it will be challenging to simply tweak it for the update, and it may need to be rewritten entirely from scratch, following the same pattern, with the more contemporary mesh API.
reactor_simulation.mp4 |
Description
This Pull Request updates the documentation and includes bug fixes for the following notebooks and follow #1486:
Motivation and Context
The motivation behind this PR is to improve the documentation and address some bugs in the mentioned notebooks. The changes made to provide more context and enhance the overall user experience.
Changes
iMeshwithpymoabBehavior
The current behavior of the notebooks is either incomplete or contains errors. This PR aims to address these issues by improving the documentation and fixing the bug in
timestep,renderand plot.Changelog file
Updated the CHANGELOG file to include a new entry describing the changes made in this Pull Request and referenced this PR number.