Feature/holidays in industrial profile #68
Conversation
birgits
commented
Aug 1, 2024
birgits
commented
- Describe your pull request as transparent as possible
- I created a new PR to address the issue raised in PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 that holiday factors should be used for holidays when setting up the industrial step load profile. I made a new one instead of building upon the existing one, because there were several issues that made it easier to start from scratch:
- PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 would have reintroduced the bug fixed in fix between_time filtering values #66, wherefore major changes to the proposed changes were necessary.
- The holiday scaling factors that were added in PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 were not used correctly, as instead the weekend scaling factors were mistakenly used.
- PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 further suggested that the electricity_demand_example.py is incorrect, which is not the case. I therefore ignored the proposed changes there.
- Tox failed in PR Fixed electricity example, added holiday functionality to industrial profile generation. #60.
- PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 was still helpful and I built upon the suggestions for the usage of separate holiday factors and used the example on how to use the industrial load profile.
- I created a new PR to address the issue raised in PR Fixed electricity example, added holiday functionality to industrial profile generation. #60 that holiday factors should be used for holidays when setting up the industrial step load profile. I made a new one instead of building upon the existing one, because there were several issues that made it easier to start from scratch:
…ar in both day_mask and night_mask
|
Thank you @maurerle for the review. I implemented your suggestions on splitting up the test. |
src/demandlib/particular_profiles.py
Outdated
| # Check for NAN values in the dataframe | ||
| if self.dataframe["ind"].isnull().any(axis=0): | ||
| logging.error("NAN value found in industrial load profile") |
There was a problem hiding this comment.
This check has been there before. It can happen when weekdays, weekend days and holidays are set inconsistently, for example when day 5 does not appear in any of the lists for "week", "weekend" or "holiday". I can't think of any other constellation leading to NaN values, but maybe it can also happen due to something else.
There was a problem hiding this comment.
I see. Maybe just don't add the comment. As you added it, I assumed that you were knowledgeable about possible errors. In fact, the error code is not included to the unit tests.
PS: What happens can be seen in the code. Comments should tell, why something is done or useful. So, this comment is not needed anyway.
There was a problem hiding this comment.
Building on your comment I wanted to add a test for the NaN error raising but realised, that NaN values are not really possible, as we initialise the dataframe column "ind" with zeros. I can't think of a case that could lead to NaN values, so we could also take out the entire NaN check. What do you think?
There was a problem hiding this comment.
I would have suggested the same but did not really know if there might be cases where NAN may occur. If its hardly possible, we could delete the check. (Should be in another PR, however. It does not have to do with improving the profiles. Maybe, we leave it in for after the release.)
There was a problem hiding this comment.
Thank you for the quick replies :) I took out the NaN check.
…emof/demandlib into feature/holidays_in_industrial_profile
