Remove usage of _result_properties dictionary #918
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
==========================================
- Coverage 85.16% 85.11% -0.05%
==========================================
Files 52 52
Lines 5217 5220 +3
==========================================
Hits 4443 4443
- Misses 774 777 +3 🚀 New features to boost your workflow:
|
| assert field.component_count == 1 | ||
| assert np.allclose(field.data, field_ref.data) | ||
| # the model does not contain structural temperature results | ||
| with pytest.raises(ValueError) as excinfo: |
There was a problem hiding this comment.
@FedericoNegri so this test used to silently fail at finding the native location, and returned something wrong?
There was a problem hiding this comment.
In this case the test was comparing trivial arrays, there's an internal bug report for that operator. But in general the logic before was resorting to some defaults in case of missing location. That resulted in hidden errors for results that weren't registered in the _result_properties dict.
| "B_T1", | ||
| "B_T2", | ||
| ]: | ||
| res = next((r for r in available_results if r.operator_name == "SMISC"), None) |
There was a problem hiding this comment.
@FedericoNegri so all SMISC results have the same native location then?
There was a problem hiding this comment.
yes, that's my understanding
| if res is not None: | ||
| return res.native_location | ||
|
|
||
| raise ValueError(f"Result with base name '{base_name}' not found.") |
There was a problem hiding this comment.
| raise ValueError(f"Result with base name '{base_name}' not found.") | |
| raise ValueError(f"Unknown native location for result with base name '{base_name}'.") |
There was a problem hiding this comment.
Either we make this check explicit here, or we move the check for existing result with base name somewhere above.
This function checks for the native location, not whether the result exists.
There was a problem hiding this comment.
This function checks for the native location, not whether the result exists.
I disagree, the function throws if it can't find the result, which is a prerequisite for querying its location.
Unknown native location would be an appropriate error message for a native location set to None.
we move the check for existing result with base name somewhere above
I've been thinking about this and considered having a function
def _get_available_result(
available_results: list[AvailableResult], base_name: str
) -> AvailableResult:
res = next((r for r in available_results if r.operator_name == base_name), None)
# special case for beam results, which are extracted from SMISC
if res is None and base_name in [
"B_N",
"B_M1",
"B_M2",
"B_MT",
"B_SN",
"B_EL",
"B_T1",
"B_T2",
]:
res = next((r for r in available_results if r.operator_name == "SMISC"), None)
if res is None:
raise ValueError(f"Result with base name '{base_name}' is not available.")
return resbut I think it would just be misleading. You come in with B_N (axial force) and it returns you an available result with physical name 'Elemental Summable Miscellaneous Data', no unit etc.
The proper fix is to have those beam results be listed as available results (that was discussed internally).
There was a problem hiding this comment.
I agree on the proper fix.
Let's call this a quick fix then.
| # the model does not contain structural temperature results | ||
| with pytest.raises(ValueError) as excinfo: | ||
| _ = transient_simulation.structural_temperature(set_ids=[1]) | ||
| assert "not found" in str(excinfo.value) |
There was a problem hiding this comment.
| assert "not found" in str(excinfo.value) | |
| assert "Unknown native location" in str(excinfo.value) |
| create_operator_callable: Callable[[str], Operator], | ||
| average_per_body: bool, | ||
| ): | ||
| res = _result_properties[base_name] if base_name in _result_properties else None |
There was a problem hiding this comment.
You could also have a function here which redirects from B_N and whatnot to SMISC as you did, but does not handle getting the native location.
This way the handling of the native location remains the same, and you have a clear function which checks for availability of results and throws a precise error when it fails.
3 participants
Stop using the static
_result_propertiesdict from PyDPF-Core and rather look up result location from the result info.What's problematic: for operators not listed as available results, we fail to lookup the corresponding native location. That's the case for beam operators (like
B_N,B_M1, etc.) that are sort of a wrapper around SMISC.I've tried to make it explicit in
_get_native_location, where I'm also throwing an exception if a result was not found, rather than ignoring it and then deliver wrong results (because averaging is misconfigured etc.).