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
Alfoa/hybrid model for batching and ensemble model #2322
Alfoa/hybrid model for batching and ensemble model #2322
Changes from 13 commits
1879ae4
1b78cd1
fafedbb
e8c57bb
d18282a
b09eacc
7114f63
b131c8a
2e94d8c
a332bd0
648c163
135bcf7
1084d1e
3269196
ab91508
5230636
09d5b63
e648a1d
d7cab4d
2cf8dd2
9c4aede
71bc89c
b25a176
7067497
40ad61b
8d4a3f5
691547e
5527d23
c81ffea
76e91c3
64611a5
5193b22
8113d33
31f466f
bf652c5
751f751
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.
very ugly :(
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.
It would be slightly less ugly if you used a set:
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.
done
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.
It is ugly and in addition this creates a sub-working directory even if, for example, the Logical/hybrid models do not use a
Code
. In case of HybridModel/LogicalModel using only ExternalModels/ROMs, the subdirectory is created but will stay empty. Not very elegant. @joshua-cogliati-inl @wangcj05 any ideas on how to improve 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.
@wangcj05 @joshua-cogliati-inl any ideas for this? I cannot find a better solution at this stage
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.
Can we have a class attribute to indicate if there is a code associated with the Model? For example, In Hybrid Model/Logical/Ensemble Model, we define a
self._isCodeAvail
, and assign it to true when we detect a code in the Model. @alfoaThere 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.
Hm, I am trying to fully understand why you need to create the directory? Just to check, it is needed if the Logical/hybrid models use a Code? (Congjian's idea sounds reasonable)
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's why. If there is a Code in the underlying Logical/Hybrid model (contained in the ensemble model) the subfolder is required.
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.
@wangcj05 @joshua-cogliati-inl can you tell me how exactly you would like that flag to be coded? (I prefer not to take a code design decision (that might be needed to be modified) on my own)?
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.
@wangcj05 @joshua-cogliati-inl FY: if you can send feedbacks within tomorrow I can try to address them before leaving on Friday. Otherwise it will need to wait till September.
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 would suggest we add these lines in Logical model and Hybrid model.
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 addressed this as well.
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.
maybe should be replaced by
issubclass(self.modelsDictionary[modelName]['Instance'], HybridModelBase)
?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 you suggestion will be better 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.
done