Replies: 16 comments
-
I agree with point 3 is your list above. It's really difficult to get a gut feel for how different something is. I often get confused on why some params show If we had some hard coded logic to handle a few cases, I think a standard relative percent might serve us well. A few of the cases that come to mind that we would need to consider:
If we find that we end up hardcoding a bunch of conditionals, then it might not be the right approach. Not sure. On the other hand, if we don't want to use the compare.log to show something more intuitive based on relative differences, than I think it's ok to keep it as is (or some variant thereof), but it would be really nice to have that docstring more formally documented in the docs. It's a lot easier to read something formally written up in the docs than remembering which method within a given class in a specific file (that might move) has the relevant docstring (recognizing that I should really heed my own words in regard to the axial expansion changer 😄). Thoughts? |
Beta Was this translation helpful? Give feedback.
-
This is handled in a way already that I think is OK. This is where you see the
This will be shown as a +/- 2. It's maybe OK how we have it now, people just need to know the math and what 2 means. It's not the most intuitive but I don't have a better suggestion.
Yes indeed. Small important diffs in one place might be ignored in favor of large unimportant diffs elsewhere. One partial remedy is education on which params get large swings in values we would rather ignore. Another is to add some kind of larger tolerance to these diff calcs (a messy suggestion to be sure!). Could perhaps be handled with a parameter setting. I don't know.
This is handled in compareDB3. The diffs calcs include the possibility for either a scalar or vector parameter: armi/armi/bookkeeping/db/compareDB3.py Lines 441 to 442 in 2be5eb9 and armi/armi/bookkeeping/db/compareDB3.py Lines 462 to 465 in 2be5eb9 |
Beta Was this translation helpful? Give feedback.
-
This is because if you give a tolerance of, e.g., 1%, the max value diff may be over that and the mean value may be under. Sometimes max and mean match, so you get both. Sometimes both mean and max are over 1%, so you get both. |
Beta Was this translation helpful? Give feedback.
-
Exactly. I want to make more rules so the compare log is more useful, but also not have a giant hardcoded specialty set of rules. |
Beta Was this translation helpful? Give feedback.
-
What if it got treated the same way as the missing params get treated? E.g.,
I like the idea of a parameter setting. Could be something like
Yeah totally. The original point was if we were to change it, what would we change it to?
Makes sense. Would be great to have this in the docs.
How many rules would we expect? At what point does it become burdensome? |
Beta Was this translation helpful? Give feedback.
-
This is totally worthwhile to consider!
Oh I think the simple calcs (mean/max of absolute differences) are OK here. I would field arguments for someone who felt otherwise, though. It's always a tradeoff! I'm thinking we could also keep a reference page for "diff quirks" that we can use internally so we aren't stuck coding up some really clunky/specific stuff in the framework. I don't know about adding much to the ARMI docs besides the basic functionality because the important differences we are discussing are plugin/reactor specific. |
Beta Was this translation helpful? Give feedback.
-
We could always just get something put together in a form that is a step in the right direction and then revisit with different choices/improvements later on. I think simplifying/improving clarity and a brief docs section would be a great first step and give us a better feel for what we want to sink our teeth into next. @opotowsky thoughts? |
Beta Was this translation helpful? Give feedback.
-
Ok so some documentation on this is definitely in order. I'm thinking in the docstring is best, then it renders here: https://terrapower.github.io/armi/.apidocs/armi.bookkeeping.db.compareDB3.html Agree? Disagree? Also, are there any changes we outlined above that you'd be comfortable implementing? I like the idea of instead of |
Beta Was this translation helpful? Give feedback.
-
That is a good place, sure. I think it would be well served in the user docs as well. Possibly a new subsection? Could be under User Docs > Accessing Data in ARMI > Comparing Two Databases. Once docstrings get to a certain length and complexity, I think it's better to just link a docs page in the docstring instead. Just my 2 cents though. I'll make a separate linked issue for replacing the +/- 2 and then whoever gets to it first can assign themselves! |
Beta Was this translation helpful? Give feedback.
-
@opotowsky Is this a ticket, or a Discussion? |
Beta Was this translation helpful? Give feedback.
-
This should be a discussion. I probably started it before I was accustomed to discussions... |
Beta Was this translation helpful? Give feedback.
-
I'd like to cast my vote to remove |
Beta Was this translation helpful? Give feedback.
-
Would it be possible to pass parameter-specific tolerance settings through a dictionary: tolerances = {
# mean abs, mean, max abs
"paramA": ( 1E-6, 1E-6, 1E-6),
"paramB": ( 1E-7, 1E-8, 1E-9),
"default": ( 1E-5, 1E-5, 1E-5),
} Then the tolerances can be grabbed like: mean_abs_tol, mean_tol, max_abs_tol = tolerances.get(param_name, "default") Of course, the code that compares the diffs will have to be updated. The tolerance dictionary could eventually be generated from the parameter system. This would be a helpful feature even if the integration with the parameter system isn't setup right away. |
Beta Was this translation helpful? Give feedback.
-
I see utility in having a tolerance dictionary, but then the user can't alter the tolerance themselves anymore. Definitely would need broad user buy-in for that. I like having the ability to toggle between 0 tolerance and N% tolerance, and it has served some important work in the past. |
Beta Was this translation helpful? Give feedback.
-
That's possible with not much more work. One possible solution: define a user-defined dictionary and a default dictionary. The two would be merged in a ChainMap. Here's a sketch of that: from collection import ChainMap
user_tolerances = {...}
default_tolerances = {...}
tolerances = ChainMap(user_tolerances, default_tolerances)
mean_abs_tol, mean_tol, max_abs_tol = tolerances.get(param_name, "default") The values specified in the user dictionary will shadow the values specified in the default dictionary. >>> from collections import ChainMap
>>> a = {"a" : 1, "b": 2}
>>> b = {"c" : 1, "b": 3}
>>> c = ChainMap(a, b)
>>> c["a"]
1
>>> c["b"]
2
>>> c["c"]
1 If a user wanted to skip the evaluation of a parameter the tolerances could be set to infinity. Some helper functions could be defined to clean-up the interface: import numpy as np
def skip_eval(user_tolerances, parameter):
user_tolerances[parameter] = (np.inf, np.inf, np.inf)
def skip_evals(user_tolerances, parameters):
for parameter in parameters:
skip_eval(user_tolerances, parameter)
def skip_all_evals(user_tolerances):
skip_evals(user_tolerances, default_tolerances.keys()) |
Beta Was this translation helpful? Give feedback.
-
The DB difference calcs are all done like so:
2 * (X - Y) / (X + Y)
This has some pros and cons, some discussed in the
DiffResults
docstring:armi/armi/bookkeeping/db/compareDB3.py
Lines 95 to 98 in 2be5eb9
I want to start a discussion on this calculation so we can handle the following scenarios better.
1. Difference is large but shows as small fraction
This is noted as an issue in the docstring. This could be bad since someone could miss this easily, thinking something isn't impacting a parameter.
2. Bounds SHOULD be +/- 2 but some diff values are much larger
If a negative value becomes positive or vice versa, the diff will be much larger than 2 in magnitude. This is an edge case we should filter for since we sometimes allow negative values even though they are non-physical.
3. There is a lot of confusion around what a difference value represents.
Ultimately, I would like all "normal" diffs to show as a fraction or a percent, and all "edge cases" to be better indicated as such (i.e.,
src
is 10 andref
is 0 will always give a diff of 2, but that isn't obvious upon seeing the number 2).Maybe we decide to change the calculation, or maybe we just institute better filtering. I don't know. I am looking for opinions!
Beta Was this translation helpful? Give feedback.
All reactions