-
Notifications
You must be signed in to change notification settings - Fork 493
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
allowing geometry to be fully made via constructor #2602
allowing geometry to be fully made via constructor #2602
Conversation
openmc/geometry.py
Outdated
**kwargs : dict, optional | ||
Any keyword arguments are used to set attributes on the instance. |
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.
For two parameters, this almost doesn't feel like it's worth it over just explicitly adding merge_surfaces
and surface_precision
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 believe @eepeterson was keen on getting all the classes fully made from constructors as part of the repr, str and then xml to python code idea. But admittedly I could have misunderstood
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.
Sorry if I was not clear. I meant rather than adding the catch-all **kwargs
as an argument, explicitly add merge_surfaces
and surface_precision
arguments. So @eepeterson's goal can still be attained.
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 sorry, you were clear I just miss read. I changed to explicitly adding these args
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 this @shimwell! To follow on to @paulromano's comment: generally I think **kwargs
should be used for pass through arguments to the superclass if necessary, whereas explicit keyword arguments are best for the class itself (especially if they are small in number).
openmc/geometry.py
Outdated
@@ -39,11 +39,11 @@ class Geometry: | |||
|
|||
""" | |||
|
|||
def __init__(self, root=None): | |||
def __init__(self, root : openmc.UniverseBase = None, merge_surfaces: bool = False, surface_precision: int = 10): |
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 we have a policy on how to write method/constructor signatures with type annotations yet? With type annotations (and default values in particular) I find this style a bit hard to read unless there are only 1 or 2 parameters. Since this line seems to be getting a bit longer than the 90ish rule, perhaps 1 argument per line, but I'd be curious to hear other perspectives. I think my preference is one per line with whitespace up the the ( but there are good arguments for other styles out there
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've changed the code to multi line args and it does look better imo but I'm always keen automate this stuff and adopt whatever flake8 or black decides
Co-authored-by: Paul Romano <[email protected]>
Hoping to contribute to @eepeterson mission of being able to entirely make the classes through the constructor. I thought I should have a go at allowing the geometry class to be specified through the constructor.
Checklist
- [ ] I have run clang-format on any C++ source files (if applicable)I have added tests and run pytest with the
pytest tests/unit_tests/test_geometry.py
as the openmc.Geometry is the class that has been changed.