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

Skip appliance if func_time==0 or rand_time==0 #116

Merged

Conversation

JW-Kraft
Copy link
Collaborator

@JW-Kraft JW-Kraft commented Feb 29, 2024

Fix #114

Implemented fix as proposed in issue 114:

  • add rand_time!=0 as condition in the while loop of function Appliance.generate_load_profile. This way, appliances are skipped when the rand_time is calculated to be 0, which can happen when time_fraction_random_variability is set to 1.

One small addition:

  • first check if self.func_time==0 (line 1803) -> if so, appliance is skipped directly, without calculating rand_time. Otherwise, Appliance.rand_total_time_of_use can throw error.

@Bachibouzouk Bachibouzouk changed the title Fix issue 114. Skip app if func_time or rand_time==0 Skip appliance if func_time==0 or rand_time==0 Feb 29, 2024
@Bachibouzouk Bachibouzouk linked an issue Feb 29, 2024 that may be closed by this pull request
@JW-Kraft JW-Kraft marked this pull request as ready for review February 29, 2024 13:23
@Bachibouzouk
Copy link
Collaborator

Bachibouzouk commented Mar 11, 2024

To be able to checkout locally @JW-Kraft branch I did

git fetch origin pull/116/head:<whichever name I want to have locally>

We only have read acces unless @JW-Kraft grant us write access like described in the docs


# Generate test user and appliance
user = User(user_name="Test User", num_users=1)
appliance = user.add_appliance(
Copy link
Collaborator

Choose a reason for hiding this comment

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

normally you could simply create an appliance without user or usecase and call generate_load_profile

Copy link
Collaborator

@Bachibouzouk Bachibouzouk left a comment

Choose a reason for hiding this comment

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

Thanks for submitting a fix for this @JW-Kraft !

@Bachibouzouk
Copy link
Collaborator

@FLomb - are you ok with merging without having the testpypi test being able to run? (the problem there is credentials of test-pypi)

@FLomb
Copy link
Contributor

FLomb commented Mar 15, 2024

@FLomb - are you ok with merging without having the testpypi test being able to run? (the problem there is credentials of test-pypi)

I'm not following, why do we have credentials issues with test-pypi?

@mohammadamint
Copy link
Collaborator

@FLomb - are you ok with merging without having the testpypi test being able to run? (the problem there is credentials of test-pypi)

@Bachibouzouk the issue is very similar to https://github.com/orgs/community/discussions/57621

@Bachibouzouk Bachibouzouk force-pushed the fix/func_time_zero_error branch from 40c03bb to 0c65148 Compare March 18, 2024 11:42
@Bachibouzouk
Copy link
Collaborator

Bachibouzouk commented Mar 18, 2024

@FLomb - are you ok with merging without having the testpypi test being able to run? (the problem there is credentials of test-pypi)

@Bachibouzouk the issue is very similar to https://github.com/orgs/community/discussions/57621

@mohammadamint - can it be that because the PR is based form a fork, the permissions are not sufficient to run test-pypi deploy test? Because I did merge your fix and update the PR

@mohammadamint
Copy link
Collaborator

@FLomb - are you ok with merging without having the testpypi test being able to run? (the problem there is credentials of test-pypi)

@Bachibouzouk the issue is very similar to https://github.com/orgs/community/discussions/57621

@mohammadamint - can it be that because the PR is based form a fork, the permissions are not sufficient to run test-pypi deploy test? Because I did merge your fix and update the PR

This seems to be the issue. A solution could be to instead of triggering the workflow on pull requests, trigger it on tag or release creation events. Do you see any problem with this solution @Bachibouzouk ?

@Bachibouzouk
Copy link
Collaborator

Do you see any problem with this solution @Bachibouzouk ?

No I think what you propose make sense

@mohammadamint
Copy link
Collaborator

mohammadamint commented Mar 18, 2024

Do you see any problem with this solution @Bachibouzouk ?

No I think what you propose make sense

There is one problem, and that is that the workflow wont be triggered on PR but on push event. I would ignore the issue for now, and proceed with this PR, if urgent, so meanwhile I can seek for a better solution.

@FLomb
Copy link
Contributor

FLomb commented Mar 18, 2024

Do you see any problem with this solution @Bachibouzouk ?

No I think what you propose make sense

There is one problem, and that is that the workflow wont be triggered on PR but on push event. I would ignore the issue for now, and proceed with this PR, if urgent, so meanwhile I can seek for a better solution.

It looks like all tests pass now; does this mean you have implemented a solution to the test PyPi issue?

@Bachibouzouk
Copy link
Collaborator

It looks like all tests pass now; does this mean you have implemented a solution to the test PyPi issue?

They all passed because their number reduced from 3 to 2 tests, after the changes introduced by @mohammadamint and approved by me. I am not sure I got why it did not work, does it only work when I push to this PR now?

@mohammadamint
Copy link
Collaborator

It looks like all tests pass now; does this mean you have implemented a solution to the test PyPi issue?

They all passed because their number reduced from 3 to 2 tests, after the changes introduced by @mohammadamint and approved by me. I am not sure I got why it did not work, does it only work when I push to this PR now?

indeed. I would however, switch back to previous version after finalizing this PR, until I can find a better solution, hopefully by the end of the week!

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.

Error when daily functioning time of an appliance is 0
4 participants