Skip to content
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

Make Node a dataclass #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

adsharma
Copy link
Contributor

Dataclasses make the code more compact and generate many convenience
functions.

@@ -520,11 +520,17 @@ def fr_load(problem: pulp.LpProblem, fr: float) -> pulp.LpAffineExpression:

if x in x_to_read_quorum_vars:
vs = x_to_read_quorum_vars[x]
x_load += fr * sum(vs) / self.node(x).read_capacity
xread_capacity = self.node(x).read_capacity
if xread_capacity is None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code would not be necessary if we made:

read_capacity: float = 1.0

in the dataclass. More instances below.

@mwhittaker
Copy link
Owner

Ah, I've been targeting Python 3.6, and I think dataclasses are a 3.7+ thing?

@adsharma
Copy link
Contributor Author

adsharma commented Apr 5, 2021

Backports are available: https://pypi.org/project/dataclasses/

In general, dataclasses eliminate so much boilerplate code that I use them wherever I can.

Copy link
Owner

@mwhittaker mwhittaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I spent some time reading up on dataclasses, and I think the switch here may not be worth it.

  • I don't think we benefit much since there's not a lot of boilerplate to remove in this case.
  • The dataclass also introduces capacity into Node which it didn't previously have, and it's not clear what capacity means. For example, if we construct a node like Node('a', read_capacity=100, write_capacity=200), there's not a nice value for capacity.
  • Also since Node is part of the Expr hierarchy, it feels weird to me that Node would be a dataclass but not And or Or.

quoracle/expr.py Outdated
write_capacity: Optional[float] = None
latency: Optional[datetime.timedelta] = None

def post_init(self):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be __post_init__.

Dataclasses make the code more compact and generate many convenience
functions.
@adsharma
Copy link
Contributor Author

Yes - it saves only 10 lines in this diff. Not an earth shattering difference :)

I've addressed the bugs you found in the code and simplified the typing and default values.

I'll leave it here just in case you end up using dataclasses elsewhere and then want to convert this one for consistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants