-
-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refiner documentation #1786
base: main
Are you sure you want to change the base?
Refiner documentation #1786
Conversation
This Also, this failing test is a bit perplexing to me as it doesn't seem that I actually changed anything. |
One strange thing about the way that certain points get marked as being in the "keep" list is that the curvature check will only keep the point next to the point of interest if it decides to split the two intervals to the right of the point of interest. This is different from how the slope criterion check marks points. I haven't quite grasped any underlying logic for which points get added to the "keep" list during each refinement check. |
@speth I went ahead of created a new page that talks about the grid refinement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wandadars ... thank you for taking this on! While this is still work-in-progress, I wanted to provide some feedback that may be a little nit-picky, but should help with improving consistency of our documentation as well as our code base.
Edit: I am not sure why unit tests broke, but it does appear that you inadvertently changed criteria for regridding, which can hopefully be rolled back.
vector<bool> m_active; | ||
|
||
//! Refinement criteria |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This docstring only applies to the first of the subsequent member variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my vs-code editor was leading me astray there, it applied it to all the subsequent items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the case, there may be a setting I’m unaware of. The doxygen output will tell.
include/cantera/oneD/refine.h
Outdated
size_t m_nv; | ||
size_t m_npmax = 1000; | ||
|
||
Domain1D* m_domain; //! Pointer to the domain to be refined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Domain1D* m_domain; //! Pointer to the domain to be refined | |
Domain1D* m_domain; //!< Pointer to the domain to be refined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want two spaces between the semicolon and the start of the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t think we have a guideline on this. On the Python side, it’s a recommendation that I have seen, and I think it’s slightly more readable. It’s really your choice; the <
was definitely missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second space is more helpful in the Python case, where you don't already have a semicolon ending the actual code statement. The one-space version is about 15x more common in the C++ code base at present.
include/cantera/oneD/refine.h
Outdated
//! Names of components that require the addition of new grid points | ||
set<string> m_c; | ||
set<string> m_component_name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set<string> m_component_name; | |
set<string> m_componentNames; |
See C++ Style Guide. I believe that for variable names that do not appear as part of the public
interface we are not as concerned about descriptive names, which have to be balanced with readability of the implementation itself. m_c
can certainly approved upon, but m_cNames
would be a shorter compromise.
include/cantera/oneD/refine.h
Outdated
set<size_t> m_loc; | ||
set<size_t> m_insertion_points; | ||
|
||
//! Map of grid point indices that should be kept, 1=keep, -1=remove, 0=unset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//! Map of grid point indices that should be kept, 1=keep, -1=remove, 0=unset | |
//! Map of grid point indices that should be kept. Values are ... |
Detailed description should not be part of the first statement, which forms Doxygen's @brief
description. Also, which integer is what?
As an aside, using an enum
as the value instead of these sentinel values would further clarify things.
include/cantera/oneD/refine.h
Outdated
double prune() { | ||
return m_prune; | ||
} | ||
|
||
protected: | ||
//! Indices of grid points that need new grid points added after them | ||
set<size_t> m_loc; | ||
set<size_t> m_insertion_points; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set<size_t> m_insertion_points; | |
set<size_t> m_insertionPoints; |
I am torn about changing member variable names here: m_loc
is used in many other contexts within the Cantera code base where the meaning is well known. At the same time, it is a more meaningful name.
While we aren't completely consistent on this, the default naming style would be to use camelCase after the initial m_
, which in this case would yield m_insertionPoints
- see C++ Style Guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about m_insertPts ?
If it's unused, please go ahead and deprecate it. For this, we usually issue a
I disagree - some of the failing tests show
which indicates that regridding was inadvertently changed. Same goes for |
Thank you for the feedback @ischoegl . I'm still looking into how exactly I changed the algorithm. |
The source of the change that was causing the tests to fail was the change of:
Where I had just moved the m_thresh up into the max_change variable, as the purpose of that variable seems to be a protection against divide-by-zero FPEs.
The thing that I'm not sure about is why this mattered. There's the division of
The failing test is shown below. The width of the domain is 0.05m, and if there's say 100 points, then the dz is on the order of 5e-4m, which would make the effective FPE threshold something like 2.98e-5. Doing it the way I had adjusted it to, the threshold would still be 1.4e-8 for guarding against the FPEs.
|
I put some print statements into that part of the code to compare the two, and it does seem like my change makes the ratio larger than the other way. This makes sense because for a smaller value of the max_change, the m_thresh by itself would be smaller. I don't really see the logic of making the FPE guard larger though. Edit: I'm inclined to think that the old way of handling the ratio criterion was permitting un-resolved points to pass erroneously.
|
I looked at that failing test and it does look like for the case when the denominator is simply using the m_thresh, a ton of points get put in a region of almost constant slope. So maybe the guard against regions of constant slope isn't working properly causing the algorithm to be activated? I printed out all the cases where the ratio was greater than 1 using the newer ratio definition and it looks like there's a sensitivity going on where maybe the more points that are added causes noise that can make the slope vary significantly, which drives more points to be added. |
I think one issue was that the refiner was considering (for the failing test) the species Argon, which was a constant, but there were small fluctuations in the value, and because the slope criteria only tested for regions of constant slope, it entered into the slope refinement section and inserted points, which seems to have generated more noise in the slope, which generated more points. If I have understood the nature of the issue, basically the zero slope case seems to have had positive/negative values or just noise that the first if-statement catch seems to have deemed as being significant enough of a variation from a constant to let it enter the clause. Maybe a case where because the value under consideration was zero, this is different from a case of a non-zero constant slope. I put an extra clause on the slope constant-value-check clause where it just uses the previous constant-value-clause for the previous if-statement that refines based on values. Doing this removed the excessive refinement ( it was putting 3500 points in the free flame case that was failing). It returned back to putting around 100 points over the flame after adding this additional check. Looking at Argon, we have the following outputs for the free-flame test case.
Looking at the valMax-ValMin relative to the min_range*valMagnitude, it is clear this is a constant value. But looking at the slope quantity tells a different story because the magnitude of the slope is close to zero, so the noise is considered to be significant. We can also see the old ratio and new ratio have very different opinions on the ratio. The old ratio had that m_thresh term, which added just enough to the denominator so as to ignore slopes that were very close it seems. I don't think that was the intention of the original coders, as they also used the m_thresh in the previous check for the value refinement. I may be incorrect though. It seems like the expected behavior is to not let the "bad" points even get into the refinement insertion algorithm instead of letting them in and then within the algorithm ignoring ones that fail to meet that |
It appears I have done something wrong here. I thought I was rebasing my branch onto main, but this page now shows that 65 files were changed. |
It looks like maybe that m_thresh variable may have been meant as a filter for the minimum allowable difference between two small values, and therefore for two grid points that differ by the smallest allowable amount, the smallest allowable slope would be m_thresh / dz. I'll revert my change and work to make it clear that this is likely what job the m_thresh quantity is performing. |
b6751dc
to
c36aeec
Compare
I think that forward difference first derivative approximation was susceptible to noise in the solution. I tested out a simple central difference approximation (with forward/backward difference approximations for the left/right boundary points), and the excessive refinement goes away for that free flame case. The original algorithm adds 120 points I think and the one with the central difference slope approximation adds 128, so they are fairly close. I'm not going to change anything though, but in case it is looked into again in the future, this is what I was using.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1786 +/- ##
==========================================
- Coverage 73.24% 73.23% -0.01%
==========================================
Files 381 381
Lines 54375 54376 +1
Branches 9253 9254 +1
==========================================
- Hits 39826 39822 -4
- Misses 11578 11581 +3
- Partials 2971 2973 +2 ☔ View full report in Codecov by Sentry. |
This is a WIP
Mostly documentation changes, with a few variable name adjustments for clarity.
Checklist
scons build
&scons test
) and unit tests address code coverage