-
Notifications
You must be signed in to change notification settings - Fork 2
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
Serialise job (#19) #25
base: main
Are you sure you want to change the base?
Conversation
…eModel.copy. Fixed bug in threads_per_node.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 81.38% 81.66% +0.28%
==========================================
Files 54 54
Lines 3223 3267 +44
==========================================
+ Hits 2623 2668 +45
+ Misses 600 599 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Very nice and mostly uncontroversial. A little suggestion to make/keep the API coherent, otherwise GTG from me.
@@ -48,52 +48,51 @@ def threads_per_node(self): | |||
The number of logical cores per node (threads). This value is automatically derived | |||
from the above properties. | |||
""" | |||
return self.cores_per_node * self.threads_per_core | |||
return self.cores_per_node() * self.threads_per_core |
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 think the API would be nicer if we declared cores_per_node
(and for consistency) threads_per_node
as properties. Then the look and feel of reading Job.threads_per_core
and Job.threads_per_node
is the same, even though one is derived from the other (which is an implementation detail that a user doesn't need to care about).
Renamed copy to clone to avoid clash with pydantic.BaseModel.copy. Fixed bug in threads_per_node.