-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add units to ResultsIndex #330
Conversation
Codecov Report
@@ Coverage Diff @@
## master #330 +/- ##
==========================================
+ Coverage 82.68% 82.72% +0.04%
==========================================
Files 35 35
Lines 3170 3184 +14
==========================================
+ Hits 2621 2634 +13
- Misses 549 550 +1 |
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.
LGTM! :)
src/ansys/dpf/post/index.py
Outdated
for i, unit in enumerate(units): | ||
result_values[i] += f" ({unit})" if unit is not None else "" | ||
if len(units) < len(values): | ||
units.extend([None] * (len(values) - len(units))) | ||
else: | ||
units = [None] * len(values) |
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.
Probably we want to avoid writing "()"
if unit
is None
. :)
And personally, I read better the if
before the for
. But this is a nit pick :).
for i, unit in enumerate(units): | |
result_values[i] += f" ({unit})" if unit is not None else "" | |
if len(units) < len(values): | |
units.extend([None] * (len(values) - len(units))) | |
else: | |
units = [None] * len(values) | |
if len(units) < len(values): | |
units.extend([None] * (len(values) - len(units))) | |
for i, unit in enumerate(units): | |
result_values[i] = f" ({unit})" if unit is not None | |
else: | |
units = [None] * len(values) |
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.
Probably we want to avoid writing
"()"
ifunit
isNone
. :)And personally, I read better the
if
before thefor
. But this is a nit pick :).
Hi @germa89! Thx for the review! Just to nit pick too, the original line 174 does actually write ""
if unit
is None
;)
Also, the if
after the for
is because the operation of adding the unit to the string rep is not needed for None
values of units, which is all we are adding in the if
.
Thus having the if
before the for
might be a bit more readable, but is adds unnecessary operations. What I will do is add comments so it is more readable because you are right, it is not really readable.
ref = ( | ||
"DataFrame<index=MultiIndex<[MeshIndex<name=\"node_ids\", dtype=<class 'int'>>, " | ||
"Index<name=\"components\", dtype=<class 'str'>>]>, columns=MultiIndex<[ResultIndex<['U']>, " # noqa: E501 | ||
"SetIndex<values=[1]>]>>" | ||
"DataFrame<" | ||
"index=MultiIndex<[" | ||
"MeshIndex<name=\"node_ids\", dtype=<class 'int'>>, " | ||
"Index<name=\"components\", dtype=<class 'str'>>" | ||
"]>, " | ||
"columns=MultiIndex<[" | ||
"ResultIndex<['U (m)']>, " | ||
"SetIndex<values=[1]>" | ||
"]>>" | ||
) | ||
assert repr(df) == ref |
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.
You might want to use this pytest plugin to automatize "golden" tests
https://github.com/oprypin/pytest-golden#pytest-golden
Feel free to ping me about this.
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.
oh nice, thx @germa89, I did not know this plugin!
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.
Do you know if it works well for numerical values?
ResultsIndex
.ResultsIndex
, and thus ofDataFrame
.We should also think of manipulating UnitSystem objects from PyDPF-Core -> at the Simulation level, to switch UnitSystem.
math.unit_convert_fc
.