-
Notifications
You must be signed in to change notification settings - Fork 53
feature[frontend]: improve printing of materials and material library #2356
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
base: develop
Are you sure you want to change the base?
Conversation
5b05fae
to
afa9bb4
Compare
@@ -0,0 +1,86 @@ | |||
from typing import List |
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.
thanks for starting me off with this util.py file @yaugenst-flex. Made some minor modifications, but overall it works well!
afa9bb4
to
9fc3109
Compare
|
||
class MaterialLibrary(dict): | ||
def __str__(self): | ||
return summarize_material_library(self) | ||
|
||
|
||
material_library = MaterialLibrary( |
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.
Neat! Didn't think of this approach, but that looks great.
@property | ||
def summarize_mediums(self) -> Dict[str, Union[PoleResidue, Medium2D, MultiPhysicsMedium]]: | ||
return {} |
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.
Bit confused by this property, what's its purpose?
- Methods for pretty printing the `material_library` as well as materials and their variants via their string methods. | ||
|
||
### Changed | ||
- Updated `__repr__` for `AbstractMedium` instances that have a specified name field to use that name instead of the full string representation of the medium to reduce clutter when printing models that have these mediums. |
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 feel like this is a bit too technical for this change, maybe focus more on the impact this has on usability. Maybe something like "Named mediums now display by name for brevity; materials/variants print concise summaries including references."
def test_medium_repr(): | ||
"""Test the new repr method does not error for regular media with and without names.""" | ||
|
||
test_media = [ | ||
td.Medium(permittivity=1.5**2), | ||
td.Medium(permittivity=1.5**2, name="material"), | ||
td.material_library["SiO2"]["Horiba"].updated_copy(name=None), | ||
] |
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.
Not sure what this test does? __repr__
is never called here no?
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.
Now that I'm thinking about this some more I feel like we should do two, optionally three things:
- A concise
__str__
representation that is pretty similar to the one currently. Maybe even shorter, like just a list of the material names. Not sure. Tbh if we have the below, might also just skip it entirely for the material library and just print the dict instead. - A
.show()
,.describe()
, or.pretty_print()
(just throwing some names out there) method that prints a nicely formatted summary usingrich
. For the material library as a whole that could be e.g. a table. We already userich
in other places so this would not be an additionaly dependency. - Optionally (but definitely nice), a
_repr_pretty_
method (see here) to ensure nicely formatted output in jupyter notebooks too. I think this can largely reuse whatever we do in 2.
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.
These are mostly smoke tests, which are good to have but it'd be good if we would also test some functionality, e.g. if the material names are what we think they are, whether the __repr__
returns the string, what it does for None
...
The variants and material library are a bit more annoying, we should still check if e.g. some expected strings are in there or not.
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.
Thanks @groberts-flex, this is much needed! I think the changes to the Medium
work well as-is, and thanks for adding all the material names! Regarding pretty printing within the material library itself I added some more comments because I think we should try to make this output as pretty and useful as we can if we're already working on it. But all in all it's looking pretty good!
Improving of material printing to address feature request #2307.
Output:
Printing mat:
Printing slab with mat as medium:
Printing dict holding medium: