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 128 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.
I still do not know enough about what this class actually does to approve this. The docs do not explain what this is or how to use it, and I am not sure what this code actually adds (i.e. is it even necessary?).
For one, this needs a descriptive doc string to explain what it does.
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.
@radhakrishnatg would be the best person to respond to this.
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.
Based on discussions, @radhakrishnatg explained the need for these classes, particularly with regard to cost calculations later on. @andrewlee94 are there other particulars that you want addressed, aside from further documentation?
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.
@radhakrishnatg and I have discussed this. The first big thing is some basic documentation so that users and maintainers can understand what they are looking at (this can just be better doc strings). I also pointed out that the same basic interface could be done using a more standard approach of having the users write derived classes instead of the current approach of providing a method as an argument (it adds 2 or 3 lines of boilerplate code, but aligns this tool with all the rest of IDAES).
I would also like to make sure there is a maintenance plan for this and an assigned maintainer.
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 Do you mean that we would have Design and Operation model classes as is, with the model build config options (and associated code) removed, then build model specific classes? For example, NGCC design model would inherit from DesignModel class, and it would merely add the required code under the build method to construct the model; similarly, NGCC operation model would inherit from OperationModel and add required code under build method. Is that along the lines of what you were thinking?
In the interim, we would be maintaining this code for flex desal work, but I imagine that there are other activities that would make use of this code as well, now and in the future. I guess the assigned maintainer(s) would be up for discussion. Arguably, it could be @radhakrishnatg , @MarcusHolly , and myself.
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.
Yes, that was my suggestion as it is almost identical to writing the method that is passed in as an argument.
My main reason is because this is how the other IDAES models are written and so aligns this with the standard. Otherwise we have this one section of the code that does things differently which will end up confusing someone at some point.
For the transition, you could support both - someone writing the build method just does not provide the model build config. However, you would want to add a deprecation to the
model_build
pathway and not document it (which is easy as there is no documentation yet).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 am pasting my response here for better visibility:
@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.
@andrewlee94 So I would propose the following:
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.
@adam-a-a My counter to the argument against derived classes is that from a code perspective there are only two additional lines of boilerplate code, and thus derived classes are effectively no harder that the
model_build
approach. Consider:is equivalent to:
The one difference is how arguments to the method would be handled, but even that is fairly easy. For the derived class approach, the code just needs to refer to
self.config.model_build_arguments
(which already exists) instead of expecting the mas direct arguments.I do agree that this could be done in a later PR however. The main reason this came up was that it is not particularly well documented how this was intended to be used so I wanted clarification on useage for this PR.
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.