Introduce grid time of use pricing by HarryG240799 · Pull Request #208 · CLOVER-energy/CLOVER

HarryG240799 · 2023-06-30T12:14:14Z

Description

Describe the pull request:

Why are you opening this pull request?
- Does the pull reuqest resolve an outstanding bug? If so, mark the pull request with the bug tag.
- Does the pull request introduce new features to CLOVER? If so, mark the pull reuqest with the feature tag.
What version of CLOVER will this be merged into, and what version will it be updated to? NOTE: if you are updating the version of CLOVER, please update the various metadata files to reflect this.

Linked Issues

This pull request:

closes issue 1,
resolves issue 2,

Unit tests

This pull request:

modifies the module unit tests for modules X and Y,
introduces new component unit tests for the Z component.

Note

Any other information which is useful for the pull request.

Requirements

Reviewers

All pull requests must be approved by an administrator of the CLOVER-energy organisation. Make sure to request a review or your pull request will not be approved.

Checks

CLOVER runs a series of automated tests. Run the ./bin/test-clover.sh helper script to run these prior to opening the pull request. You will not be able to merge your pull request unless all of these automated checks are passing on your code base.
NOTE: If you are modifying the automated tests, be sure that you justify this.

Metadata files

If you are opening a pull request that will update the version of CLOVER, i.e., bring in a new release, then you will need to update the various metadata files as part of your pull request:

.zenodo.json - Update the version number, author list, and date of your proposed release. Add any papers which have been released relevant to CLOVER since the last release if relevant;
CITATION.cff - Update the version number, author list, and date of your proposed release. NOTE: the date will need to reflect the date on which your pull request is approved;
setup.cfg - Update the version number of CLOVER and include any new files or endpoints required in the clover-energy package:
- The version is updated under the version variable,
- New packages should be added under the install_requires list,
- New endpoints should be added under the console_scripts list;
src/clover/__main__.py - Update the __version__ variable name to reflect these changes internally within CLOVER.

BenWinchester

General thoughts

It looks like some of the code is doing what you imagine it should, but sections of mains_supply:__utils__.py I don't think are doing what you think? and the logic isn't that clear at the minute to me. (This may just need more comments!)

📝 Update description

It would be good to update the description at the top and fill in the information that it's asking for. E.G., "why are you opening this pull request?" (maybe, "to introduce new features into CLOVER for grid time-of-use pricing" or something similar?) etc.

Automated testing

You'll see at the bottom of the review page that there are some automated tests which are currently failing. You can run these locally on your computer, or you can use the outputs of the tests to inform changes, but, in general, your code should:

be formatted using the black python formatter,
pass mypy and pylint which type-check and lint your code,
and not fail any of the automated integration testing 😄

BenWinchester · 2023-06-30T13:50:52Z

                    status.append(0)
        times = pd.DataFrame(status)
+
+        timesdaily = times % 24


❓ I don't think this is doing what you think...

What are you trying to do with this bit of code? I'm not able to follow the logic, so you may want more comments in here...

times is a dataframe with hours from 0 to the end of the simulation period in its index column, and the availability of the grid in the other column by the looks of it. For the grid called example, it looks something like this:

0 0 1 0 2 0 3 0 4 0 5 0 6 0 7 0 8 1 9 1 10 1 11 1 12 1 13 1 14 1 15 1 16 1 17 1 18 0 19 0 20 0 21 0 22 0 23 0

so, by using the remainder operator, %, all the values will have the same remainder when divided by 24:

>>> set(pd.DataFrame(times % 24 == times)[0].values) {True}

I.E., this code doesn't seem to be doing much...
2. By having a probability of availability, and then extending the outage time in this way, your availbility probabilities won't be accurate. E.G., let's say the grid is to be unavailable for 12 hours when there's a blackout. If you simply set the 11 hours following a blackout to be zero, and the grid was originally avilable 23 hours of the day (let's say), then it'll now be available for just 12 hours of the day, so the average avilability will be half what you input. I feel this is misleading (assuming this is what you're trying to do?).

BenWinchester · 2023-06-30T13:54:47Z

+
+# Append additional rows with a value of 0 to df1
+
+        for i, n in zip(zero_indices, n_values):


❓ Unclear what you're doing here

This is mostly due to variable names:

n - is this... the number of... hours?

n_values - not sure what this is as it's just a list of ones...?
(Pdb) n_values Name 0 1 0 1 0 1 0 1 0 1 .. 0 1 0 1 0 1 0 1 0 1 Name: std, Length: 51100, dtype: int64 (Pdb) np.sum(n_values) 51100 (Pdb) len(n_values) 51100

We should chat about what you're trying to do here as I'm not too sure...

BenWinchester · 2023-06-30T15:55:30Z

+                    new_df.iloc[i,0] = hourly_device_usage.iloc[lower ,0] * (1-fraction) + hourly_device_usage.iloc[upper:i +1,0].sum()
+
+
+        import pdb


🔥 Remove the pdb, my faul, sorry

…Harry

BenWinchester · 2023-07-17T09:39:05Z

        logger.info("Hourly usage profile for %s successfully calculated.", device.name)

+        #Retrieve device laod time
+        n = device.load_time


📝 Variable names

Would be good to have nicer variable names so it's easier to read through and understand what's going on

BenWinchester · 2023-07-17T09:40:05Z


        logger.info("Hourly usage profile for %s successfully calculated.", device.name)

+        #Retrieve device laod time


📝 Longer comment?

As this is a fairly complicated bit of logic, maybe a longer comment explaining what's going on? E.G.,

# Loop through the device utilisations and extend by the device usage time, in hours, to # create a new profile.

BenWinchester · 2023-07-17T09:47:51Z

+        outage_time.index = zero_indices
+
+        # Append additional zeros to the grid availability data frame
+        for i, n in zip(zero_indices, outage_time):


🐛 Is this a bug ❓

I'm not sure whether .iloc works in this way? Worth checking with pdb that the dataframe looks like what you expect;

Are these indicies right? I'd check by using pdb as I think starting from i+1 will fill in hours from then on, but ending at i+n+1 will likely fill in an extra hour, no?

📝 i, n -> nicer names?

E.G., outage_time and length_of_outage? It makes the next line much easier to read, for one thing:

times.iloc[outage_time + 1 : outage_time + length_of_outage + 1, 0]

BenWinchester · 2023-07-17T09:53:26Z

 #   provided.
 DEFAULT_KEROSENE_DEVICE = Device(
-    False, DemandType.DOMESTIC, 1, 0, 0, 0, 0, KEROSENE_DEVICE_NAME, 0, 0
+    False, DemandType.DOMESTIC, 1, 0, 0, 0, 0, 0, KEROSENE_DEVICE_NAME, 0, 0


🐛 Load-time for kerosene

I think the load time for Kerosene should be 1 rather than 0? It's annoying that it's so hard-baked, but there you go! 😉

…Harry

HarryG240799 and others added 4 commits June 30, 2023 10:33

Add changes to new branch

d94a193

add modifications

400b1ee

🚧 Add Harry's uncommitted changes

08aadd3

⏪ Restore default 'Bahraich' location

d07725f

BenWinchester changed the title ~~add modifications~~ Introduce grid time of use pricing Jun 30, 2023

BenWinchester requested changes Jun 30, 2023

View reviewed changes

BenWinchester added 3 commits June 30, 2023 16:48

🚧 Add loop for multiple grid-attribute files

12dc59a

Merge branch 'grid-time-of-use' into Harry

eab2773

✅ Fix bug 🐛

e35f0f8

BenWinchester reviewed Jun 30, 2023

View reviewed changes

HarryG240799 and others added 3 commits July 12, 2023 11:56

comments addressed

70be05d

Merge branch 'Harry' of https://github.com/CLOVER-energy/CLOVER into …

5aaa83f

…Harry

Merge branch 'master' into Harry

240b7bb

BenWinchester requested changes Jul 17, 2023

View reviewed changes

BenWinchester and others added 3 commits July 17, 2023 12:22

Merge branch 'master' into Harry

5a2df88

Chnages added

36c04e3

Merge branch 'Harry' of https://github.com/CLOVER-energy/CLOVER into …

3a0927e

…Harry


		# Append additional rows with a value of 0 to df1

		for i, n in zip(zero_indices, n_values):

		new_df.iloc[i,0] = hourly_device_usage.iloc[lower ,0] * (1-fraction) + hourly_device_usage.iloc[upper:i +1,0].sum()


		import pdb


		logger.info("Hourly usage profile for %s successfully calculated.", device.name)

		#Retrieve device laod time

Uh oh!

Conversation

HarryG240799 commented Jun 30, 2023

Description

Linked Issues

Unit tests

Note

Requirements

Reviewers

Checks

Metadata files

Uh oh!

BenWinchester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

General thoughts

📝 Update description

Automated testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenWinchester Jun 30, 2023

Choose a reason for hiding this comment

❓ I don't think this is doing what you think...

Uh oh!

BenWinchester Jun 30, 2023

Choose a reason for hiding this comment

❓ Unclear what you're doing here

Uh oh!

BenWinchester Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

BenWinchester Jul 17, 2023

Choose a reason for hiding this comment

📝 Variable names

Uh oh!

BenWinchester Jul 17, 2023

Choose a reason for hiding this comment

📝 Longer comment?

Uh oh!

Uh oh!

BenWinchester Jul 17, 2023

Choose a reason for hiding this comment

🐛 Is this a bug ❓

📝 i, n -> nicer names?

Uh oh!

Uh oh!

Uh oh!

BenWinchester Jul 17, 2023

Choose a reason for hiding this comment

🐛 Load-time for kerosene

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BenWinchester left a comment •

edited

Loading

📝 `i, n` -> nicer names?