-
Notifications
You must be signed in to change notification settings - Fork 10
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
Bnb/dev #141
Conversation
… inheritance ordering for wind models to remove tf_generate_wind/generate_wind
sup3r/models/abstract.py
Outdated
@staticmethod | ||
def set_model_params_wind(**kwargs): | ||
# pylint: disable=E0211 | ||
def set_model_params(self, **kwargs): |
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.
shouldnt this be a static method still?
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.
Yep lol
@@ -12,7 +12,7 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class WindGan(Sup3rGan, AbstractWindInterface): | |||
class WindGan(AbstractWindInterface, Sup3rGan): |
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! just reordering solves the inheritance priority? Great!!
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.
Yeah! Python's solution to the diamond problem!
@@ -499,6 +496,10 @@ def __init__(self, source_files, out_pattern, target_meta, heights, | |||
cache_pattern=cache_pattern, | |||
n_chunks=n_chunks, | |||
max_workers=self.query_workers) | |||
DistributedProcess.__init__(self, max_nodes=max_nodes, |
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 wow i didnt know you could just call the inherited init directly and pass in self! that makes so much sense. Nice.
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! Didn't know this worked either just figured it should lol
sup3r/utilities/utilities.py
Outdated
@@ -26,6 +26,130 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class DistributedProcess: |
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.
Suggest this goes into utilities/execution.py
or similar.... the utilities.py file is getting totally ridiculous.
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.
Yeah I figured you'd have a suggestion for where to put this. I didn't like it here either.
some refactoring - ForwardPassStrategy/Regridder -> DistributedProcess. Changed inheritance order in wind models to remove _wind methods.