Update number of shells in SimulationState #2504
Conversation
| composition = Composition( | ||
| density, nuclide_mass_fraction, atom_data.atom_data.mass.copy() | ||
| ) | ||
|
|
There was a problem hiding this comment.
This modification is just to delete the duplicated codes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2504 +/- ##
==========================================
+ Coverage 68.13% 68.20% +0.06%
==========================================
Files 166 166
Lines 13961 13972 +11
==========================================
+ Hits 9513 9530 +17
+ Misses 4448 4442 -6 ☔ View full report in Codecov by Sentry. |
79122de to
4e0d0c7
Compare
|
Unfortunately this will need a small rebase |
… shells instead of raw shells
…and W of the active shells only
eb782ce to
372e618
Compare
Rebased to upstream/master now. |
…radiationfield to be active shells only, this way one can change the v_inner on fly
| 1 - (geometry.r_inner[0] ** 2 / geometry.r_middle**2).to(1).value | ||
| 1 | ||
| - ( | ||
| geometry.r_inner[geometry.v_inner_boundary_index] ** 2 |
There was a problem hiding this comment.
Could this be geometry.r_inner_active[0]?
There was a problem hiding this comment.
Tried that but since r_inner_active[0] is overwrite by the v_inner_boundary value (which can be values in between the discrete shells), sometimes it can be a bigger value than the first element in r_middle, which cause the first w to be nan, and fails the assertion in the diluteBBradiationField.
| 1 | ||
| - ( | ||
| geometry.r_inner[geometry.v_inner_boundary_index] ** 2 | ||
| / geometry.r_middle**2 |
There was a problem hiding this comment.
Double check that r_middle is correct here (matching the active radii)
There was a problem hiding this comment.
r_middle is the raw shells, but if we would like to keep the raw shape of the shells in radiationfield, then we should use r_middle I think.
| geometry is not None | ||
| ): # check the active shells only (this is used when setting up the radiation_field_state) | ||
| assert np.all( | ||
| t_radiative[ |
There was a problem hiding this comment.
I know this fixes the issue, but we probably ought to be setting the estimators shape as the active shells from the start.
There was a problem hiding this comment.
That is actually already being taken of, since the estimators only use the active shells, the simulation_state slice the active shells from the "raw" radiation_field_state.
📝 Description
Type: 🪲
bugfix| 🚦testingUpdated the no_of_shells in SimulationState to be the number of active shells, which affect the dimensionality of csvy models when the inner boundary velocity is set to be a value in between the first and last shell.
From there, found some other similar bugs that would cause issue for csvy models that has v_inner_bounday in the middle, including:
Another modification is in DiluteBlackBodyRadiationFieldState" -- include the geometry input, so only check the assertion on dilution factor >0 for active shells (for cases that v_inner is in the middle, the raw dilution factors contains nan values.)
📌 Resources
Examples, notebooks, and links to useful references.
🚦 Testing
How did you test these changes?
☑️ Checklist
build_docslabel