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

Feature critical demand #166

Closed
wants to merge 16 commits into from
Closed

Feature critical demand #166

wants to merge 16 commits into from

Conversation

adnanalakori
Copy link
Collaborator

@adnanalakori adnanalakori commented Dec 1, 2021

Just re-define constants..
Fixes #165

By the way: once I made a PR, it shows with a new number #166 -> should be a way to keep this PR within the previous one #165?

Copy link
Collaborator Author

@adnanalakori adnanalakori left a comment

Choose a reason for hiding this comment

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

I am not sure about the last commit!

@smartie2076
Copy link
Collaborator

Just re-define constants.. Fixes #165

Hi @adnanalakori! I am not sure from the title and text what you are planning in this PR, and will have to check this from the code. You should try to use a title that is different from #165, that directly tells me what you do.

Also, "fix"/"close" are github-specific words that imply an action that github will take. They are used to close issues, ie. to adress and solve an issue. Used here, I am not sure what it will do with the other PR.

By the way: once I made a PR, it shows with a new number #166 -> should be a way to keep this PR within the previous one #165?

I think you created a new PR, because you pushed to a different branch name. The other branch is feature_critical_demand and not feature_critical_demand_. If you want to continue working on #165 you need to push onto feature_critical_demand!

Should I then even look at this?

Copy link
Collaborator

@smartie2076 smartie2076 left a comment

Choose a reason for hiding this comment

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

cbc.exe and coin-license.txt should not be pushed to the repo. I am not clear what you are planning to do...

Comment on lines +49 to +52
DEMAND_AC = "demand_ac_non_critical"
DEMAND_AC = "demand_ac_critical"
DEMAND_DC = "demand_dc_non_critical"
DEMAND_DC = "demand_dc_critical"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You now have redefined DEMAND_AC and DEMAND_DC directly after you defined it. Variables have to be unique.

Copy link
Collaborator Author

@adnanalakori adnanalakori Dec 2, 2021

Choose a reason for hiding this comment

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

Ok, got it, w'll correct it -> hoppfully it runs w/o errors

Copy link
Collaborator

@smartie2076 smartie2076 Dec 8, 2021

Choose a reason for hiding this comment

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

Will run without errors, but not nesessarily run as intended :D

Comment on lines +43 to +46
TITLE_DEMAND_AC = "title_demand_ac_non_critical"
TITLE_DEMAND_AC = "title_demand_ac_critical"
TITLE_DEMAND_DC = "title_demand_dc_non_critical"
TITLE_DEMAND_DC = "title_demand_dc_critical"
Copy link
Collaborator

Choose a reason for hiding this comment

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

TITLE_DEMAND_AC and TITLE_DEMAND_DC are defined twice, this does not work.

Copy link
Collaborator Author

@adnanalakori adnanalakori Dec 3, 2021

Choose a reason for hiding this comment

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

cbc.exe and coin-license.txt should not be pushed to the repo. I am not clear what you are planning to do...

I don't know how cbc.exe was pushed, weird...! I just received this error message "Error: No executable found for solver 'cbc'", then I copied it again to offgridders folder and re-run it.

Do you know this always why this happening ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, when you commit, by default all changes and added files will be added to your commit. You have to unselect the file from the upper box (the one over the commit message box):

grafik

@@ -709,7 +709,7 @@ def distribution_grid_ac(
return distribution


def demand_ac(micro_grid_system, bus_electricity_ac, demand_profile):
def demand_ac(micro_grid_system, bus_electricity_ac, demand_profile, demand_name):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are you planning to do with demand_name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I understand it is correct.

@@ -721,10 +721,11 @@ def demand_ac(micro_grid_system, bus_electricity_ac, demand_profile):
)

micro_grid_system.add(sink_demand_ac)
demand_name.add(sink_demand_ac)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not understand what you use demand_name for, and why you use add().

Copy link
Collaborator Author

@adnanalakori adnanalakori Dec 2, 2021

Choose a reason for hiding this comment

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

I was just planning just to merge the functions generate.demand_ac and generate.demand_dc to function generate.demand, but not sure about the structure of this function below:

def demand_ac(micro_grid_system, bus_electricity_ac, demand_profile):

Also, just trying to reach your goal mentioned in the #162, that the new demand shall be like that :

sink_demand_ac_critical = generate.demand(
       micro_grid_system, bus_electricity_ac, experiment[DEMAND_PROFILE_AC_CRITICAL], demand_name=DEMAND_AC_CRITICAL
   )

Copy link
Collaborator

@smartie2076 smartie2076 Dec 8, 2021

Choose a reason for hiding this comment

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

I was just planning just to merge the functions generate.demand_ac and generate.demand_dc to function generate.demand, but not sure about the structure of this function below

Yeah, that is a good idea! You will have to change

def demand(micro_grid_system, bus_electricity_ac, demand_profile, demand_name):

Also, just trying to reach your goal mentioned in the #162, that the new demand shall be like that :
[Code]

This is correct :)

@adnanalakori
Copy link
Collaborator Author

adnanalakori commented Dec 2, 2021

Should I then even look at this?

Hi @smartie2076, sorry it just a mistake that a slash _ was added to the feature_critical_demand, then a new PR has been opened, I was wondering of that :-) Could we delate this PR completely and keep us in the #165 ?

@adnanalakori adnanalakori deleted the feature_critical_demand_ branch January 13, 2022 14:04
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