Added a check that cells are cubes when using minimum pressure support. #363
Conversation
src/Enzo/utils/utils.cpp
Outdated
| enzo_float dz) noexcept | ||
| { | ||
| // we want to ensure the median value is within N ULPs of the other 2 values | ||
| // - a ulp is the difference between 2 adjacent floating point values |
There was a problem hiding this comment.
Instead of "- a ulp is the difference between 2 adjacent floating point values" I suggest you say the following:
A ULP, or 'Unit in the Last Place', is the difference between 2 adjacent floating point values. Since floating point calculations are not exact and decimal numbers cannot always be represented exactly, comparisons between two nominally identical numbers need to take that into account. To that end, this routine checks to make sure all values are very close to each other - within N ULPs - and thus it is checking to make sure the three values are approximately equal instead of exactly equal.
There was a problem hiding this comment.
Big picture, I was pretty confused by what consistent_cube_cellwidths was doing and had to stare at it for a while. My only real suggestion is to add comments that make it clear what's actually happening so that when somebody encounters this in a few years it's easy to understand what it's doing.
src/Enzo/utils/utils.cpp
Outdated
| // we want to ensure the median value is within N ULPs of the other 2 values | ||
| // - a ulp is the difference between 2 adjacent floating point values | ||
| // | ||
| // We'll start with N = 4. But we may want need to change that |
There was a problem hiding this comment.
Instead of "We'll start with N = 4. But we may want need to change that" I suggest you say:
We currently define approximate equality to be within 4 ULPs of the median of the three values - this can be seen in the for loop in the helper function. This may need to be changed if this tolerance ends up being too restrictive.
src/Enzo/utils/utils.cpp
Outdated
|
|
||
| enzo_float width = median(dx,dy,dz); | ||
|
|
||
| auto helper = [width](enzo_float toward) -> enzo_float |
There was a problem hiding this comment.
// Returns a value that is N ULPs below or above 'width', with N defined in the for loop.
// The value of 'toward' determines whether the number returned is below or above.
src/Enzo/utils/utils.cpp
Outdated
| enzo_float min_val = helper(-INFINITY); | ||
| enzo_float max_val = helper(+INFINITY); | ||
|
|
||
| // we intentionally use bitwise-and rather than logical-and for speed |
There was a problem hiding this comment.
// returns TRUE if all three values are within N ULPs of each other, FALSE otherwise.
|
@mabruzzo this PR also needs to be updated to the tip of |
Thanks @bwoshea for the suggestions.
|
@bwoshea Thanks for the suggestions. I threw this changeset together really quickly and didn't think about readability. I made some additional changes. Let me know what you think |
|
@mabruzzo I fixed the one typo I spotted, and I'll merge as soon as the tests pass. |
name: Matthew Abruzzo
about: Modifies
EnzoBlock::SetMinimumSupportso that we check that cells are cubes before modifying the fieldtitle: 'Added a check that cells are cubes when using minimum pressure support.'
Pull request summary
This is pretty self-explanatory. We added a check to
EnzoBlock::SetMinimumSupportto ensure that cells are cubes. This is an implicit assumption in the calculation that we were not previously checking.I had intended to add this into PR #340, but it got merged in before I had the chance.
I choose to implement this check as a utility-function since we should be making a similar check when we set
grackle_field_data.grid_dxin grackle (and we'll need to reuse this functionality). I did not implement that other check as part of this PR since we will need to take some extra care about when exactly we require cube-grid cells (I can expand on that in more detail)