-
Notifications
You must be signed in to change notification settings - Fork 21
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
GSYE-842: Provide filepath to ProfilesHandler.rotate_profile in order… #1827
Conversation
… to be used when rotating profile provided by file
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1827 +/- ##
==========================================
- Coverage 69.59% 69.58% -0.01%
==========================================
Files 150 150
Lines 14247 14250 +3
Branches 2670 2672 +2
==========================================
+ Hits 9915 9916 +1
Misses 3805 3805
- Partials 527 529 +2 |
…ct in order to fix the device statistics
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.
LGTM
profile_type: InputProfileTypes, | ||
profile, | ||
profile_uuid: str = None, | ||
input_profile_path: str = None, |
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 understand but why the extra parameter here? If I recall correctly, this is exactly the reason why we allow str
as the type of the profile
parameter, in order to be able to support file paths. Did you try setting the profile path to the profile
parameter and it did not work out?
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.
Please have a look into the StrategyProfile. If the profiles was already read once, the profile will be the profile for he first week. This will be used by the read_arbitrary_profile which will re-use it, but even worse: it will convert it again into kWh. The latter resulted in very low energy demands that are close to or lower than our Floating_point_tollerance.
I am open to better solutions 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 think that you can use the input_profile
in the StrategyProfile
class. Let me leave a possible suggestion that you can also double check if it works out.
@@ -128,7 +128,10 @@ def read_or_rotate_profiles(self, reconfigure=False): | |||
profile = self.profile | |||
|
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 needs to be checked whether it violates the previous if clauses, however the solution can be similar to this, so that the profile variable is overridden if we need to rotate a profile that originates from a CSV file:
if isinstance(self.input_profile, str): | |
profile = self.input_profile |
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.
if isinstance(self.input_profile, str):
will always we True for all the market slot in the simulation which will result in calling the read_arbitrary_profile inside the ProfilesHandler on every market slot, which is not what we want.
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.
Grrrr, you are right, I just saw the should_create_profile
. I agree with you then that it is ok to bite the bullet and deal with the extra parameter. Thanks for bearing with me!
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.
LGTM
… to be used when rotating profile provided by file
Reason for the proposed changes
Please describe what we want to achieve and why.
Proposed changes
INTEGRATION_TESTS_BRANCH=master
GSY_FRAMEWORK_BRANCH=master