Skip to content
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

Small fixes before the release #152

Merged
merged 15 commits into from
Jun 7, 2024
Merged

Small fixes before the release #152

merged 15 commits into from
Jun 7, 2024

Conversation

Bachibouzouk
Copy link
Collaborator

@Bachibouzouk Bachibouzouk commented Jun 5, 2024

  • fix the conversion from .py to .xlsx
  • fix the link to contribution.html
  • fix test_run.py
  • fix unexpected warning on appliances with duty cycles

@Bachibouzouk Bachibouzouk changed the title Fix ramp convert Small fixes before the release Jun 5, 2024
@Bachibouzouk Bachibouzouk changed the base branch from main to development June 5, 2024 13:45
@Bachibouzouk
Copy link
Collaborator Author

Contributing.html is browsable locally with the docs but somehow not online, I need to have a closer look at it, somehow commiting the .md file did not solve the issue

@FLomb
Copy link
Contributor

FLomb commented Jun 5, 2024

Hi,

I've pushed a commit that fixes the path issues in test_run.py, as long as the file is run from the root folder, as indicated in our contributing guidelines. So that's done. I've also introduced a random_seed=True parameter, which may be matched with reference outputs also generated via random seed in future upgrades of the code.

Nonetheless, in implementing such a fix, I bumped into other small issues. Namely, I get the following warning for all three input files:
The app has duty cycle option on, however the switch on event fell outside the provided duty cycle windows

The warning poses 2 issues:

  1. The warning string does not work as expected, because it's supposed to provide the appliance name (in the code, the string is: f"The app {self.name} has duty cycle...) and it does not do so;
  2. The warning should not be there in the first place, I suppose. So this warning is either triggered unnecessarily, or something is wrong in the input files (which I doubt).

@Bachibouzouk
Copy link
Collaborator Author

Bachibouzouk commented Jun 5, 2024

  1. The warning string does not work as expected, because it's supposed to provide the appliance name (in the code, the string is: f"The app {self.name} has duty cycle...) and it does not do so;

The app name seem to be "", now I added a default name when an appliance gets added to a user in #153 (specifically 522b436)

@Bachibouzouk
Copy link
Collaborator Author

2. The warning should not be there in the first place, I suppose. So this warning is either triggered unnecessarily, or something is wrong in the input files (which I doubt).

I found the following comment in the code

                    # TODO previously duty_cycle3 was always considered as default if the evaluate proxy did neither
                    #  get selected by duty_cycle1 nor duty_cycle2, for default is kept but not silently anymore in
                    #  order to see whether this is an issue or not
``

@Bachibouzouk
Copy link
Collaborator Author

2. The warning should not be there in the first place, I suppose. So this warning is either triggered unnecessarily, or something is wrong in the input files (which I doubt).

@FLomb - the warning was actually showing some odd behavior which was swept under the rug before, see #153 (it does not modify tremenduously the example profiles, fortunately :))

Bachibouzouk and others added 7 commits June 6, 2024 14:21
Previously the mean of the index range was computed and see if it was
falling within a duty cycle. This method compare the boundaries of the
range with the one of each duty cycle to know if there is an
intersection. If there is one, the duty cycle is chosen.

Note: this method does not take into account the case where the indexes
range spans accross more than one duty cycle.
@Bachibouzouk Bachibouzouk requested a review from FLomb June 6, 2024 22:05
Copy link
Contributor

@FLomb FLomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested extensively the PR locally, and all seems to work very smoothly. I am happy to approve it. Thanks, @Bachibouzouk !

@Bachibouzouk Bachibouzouk merged commit 077ddbe into development Jun 7, 2024
3 checks passed
@Bachibouzouk Bachibouzouk deleted the fix/ramp_convert branch June 7, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants