Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Price taker model for DISPATCHES, Rehashed #1358
base: main
Are you sure you want to change the base?
Price taker model for DISPATCHES, Rehashed #1358
Changes from 133 commits
a3d688c
67ce6a2
e716c36
e3788fb
8ef54be
f154700
422d0ba
2f36bbb
0aa1034
9042e5c
5549028
2fde47f
d0a6c13
e5f6c07
56d9369
9b5edd4
948f40c
30fed63
fc43a3f
4170501
1d87aea
4710a10
be68154
66db0d8
b39f4d9
799ed0b
7625d07
0c2c461
16710a6
4518240
35ddead
fd15af9
b84ea43
15ea641
53e427e
14ee544
4e10801
d68d8fd
6cd966e
f7e3245
b223cf6
56f8099
48d4d98
fde3f31
5bd0d5b
0df8b45
fd595b4
a54c3ac
70cbaba
85b7bd0
9851b88
6ede14e
d31b272
769ba7b
0bba4d2
b06d467
0cf82bf
6af8c9e
1fc5f53
e991392
f2725cc
522023b
22dbe12
6dbb183
7a86d03
7a24fba
b1629eb
d193e65
fb543c7
460af99
c90bbe1
643707f
8aa02c6
323e4a9
04d07ae
4ae5779
893743d
82f8bdc
ebdc1b2
aad41fb
e2dac86
5ad727e
2ae4635
8762f7b
7110dea
8550569
69c3c9b
177618c
ce4b0fb
343d191
cec302e
652d1cc
d8ade5d
cd3f3d3
ce6b3db
f4cfbdb
cbd7aaa
f10ea59
46a3331
7bdd94d
1c85981
6956d73
8b4b479
646b88b
a1f1eb2
8101448
d8de0dd
fc7369e
75a56f4
df687af
47b66c5
8ac5987
d10b9a7
1e7842a
8b1393a
6481545
b9f3ef4
bd2db6f
9cf4a3a
112761f
fb0a05b
5d960a4
d7a0002
727be78
aebe5b8
411bb60
bd3244c
b8149b5
043ac34
2cd22e4
e9ff144
422ec6e
042a1d9
0fd8edb
892e555
a2c97c4
20db58f
a4620de
a1ae6b7
bef17fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why are these necessary? It is generally bad practice to do blanket disabling of pylint warnings, and by doing this I cannot see what was the cause of the warning to suggest how to fix it.
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 agree with @andrewlee94. My suggestion:
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.
What is the expected behaviour/usage in this case? I note that if this occurs then none of the following code will run, and I do not see any documentation or messages about what this would mean for the user.
I think a logger message is required here at the least.
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 agree that we should add a logger message here.
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 addressed this already, but feel free to comment on the warning itself
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.
Similarly, the usage of this class will need to be clearly documented, especially what
model_func
is expected to do. I am again wondering if it would be better to get the user to inherit from this and just writemodel_func
as theirbuild
method.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.
@andrewlee94 I was just thinking about this, and on the one hand, using derived classes would be more in line with how the rest of IDAES does things. On the other hand, using this
model_func
approach gives the potential of applying either approach: I can formulate quick and dirty models on the fly by using the config option to pass in a build function, or I can go and create a child class. So in a sense, allowing for the current approach would give more flexibility, while eliminating it would be more restrictive on the user (which might be the way to go in most cases, but not necessarily here).In any case, I think that any refactor of this formulation should be part of a subsequent PR. I think it is more important for this overall capability to get merged in first, since there has already been published work on this Pricetaker approach (for one).
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.
This approach is backwards. If we know a refactor is coming, it's better to do it before the PR is merged. Otherwise we need to deprecate the old class, leave it in a few cycles, and create a similarly-named new class in order to maintain backwards compatibility.
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.
In fact, we don't know a refactor is coming and it is something that is really up for discussion at this point--which is why I said "if any refactor." This PriceTaker class has been delayed for quite some time now, and I don't think we should wait until the discussion settles.