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

Clover hpc to merge #27

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

sheridanfew
Copy link

Proposal to merge hpc-enabling changes back into the master branch as discussed

…oblems with some compilers.

Clean up anomylous naming and spacing. This is in order to allow automatic editing of scripts matching given patterns in later changes.

Used the following commands to achieve this in shell terminal in CLOVER directory:

# Clean up inconsistency in type of inverted commas. This has caused problems with some compilers.

SRC="‘"
DST="'"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py
SRC="’"
DST="'"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

# Clean up anomylous naming and spacing. This is in order to allow automatic editing of scripts matching given patterns in later changes.

sed -i "" -e "s/location_input_data/location_inputs/g" Scripts/*/*py
sed -i "" -e "s/_inputs =  pd.read_csv/_inputs = pd.read_csv/g" Scripts/*/*py
sed -i "" -e "s/_inputs  = pd.read_csv/_inputs = pd.read_csv/g" Scripts/*/*py
Reconfigure to take '.' as filepath - necessary for HPC where CLOVER is copied to the node before processing. (May wish to amend if CLOVER dir gets very big)

Commands used:

SRC="\/\*\*\*YOUR LOCAL FILE PATH\*\*\*\/CLOVER 4.0"
DST="."
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

SRC="\/\*\*\*YOUR LOCAL FILE PATH\*\*\*\/CLOVER"
DST="."
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

# Reconfigure to take kwargs for location
sed -i "" -e 's/__init__(self)/__init__(self,**kwargs)/g' Scripts/*/*py

SRC="self.location = 'Bahraich'"
DST="self.location = kwargs.get('location')"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

SRC="self.location = ‘Bahraich’" # This is required too as there is some inconsistency in type of inverted commas used in original CLOVER scripts.
DST="self.location = kwargs.get('location')"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py
# Reconfigure to take '.' as filepath - necessary for HPC where CLOVER is copied to the node before processing. (May wish to amend if CLOVER dir gets very big)

Bash commands:

SRC="\/\*\*\*YOUR LOCAL FILE PATH\*\*\*\/CLOVER 4.0"
DST="."
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

SRC="\/\*\*\*YOUR LOCAL FILE PATH\*\*\*\/CLOVER"
DST="."
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py
# Reconfigure to take kwargs for location
sed -i "" -e 's/__init__(self)/__init__(self,**kwargs)/g' Scripts/*/*py

SRC="self.location = 'Bahraich'"
DST="self.location = kwargs.get('location')"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

SRC="self.location = ‘Bahraich’" # This was required too as there was some inconsistency in type of inverted commas used in original CLOVER scripts - may have been fixed by earlier commands
DST="self.location = kwargs.get('location')"
sed -i "" -e "s/$SRC/$DST/g" Scripts/*/*py

# Set up dependencies in taking kwargs for location

sed -i "" -e "s/Diesel()\./Diesel(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/Grid()\./Grid(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/Solar()\./Solar(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/Finance()\./Finance(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/GHGs()\./GHGs(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/Load()\./Load(kwargs)./g" Scripts/*/*py
sed -i "" -e "s/Energy_System()\./Energy_System(kwargs)./g" Scripts/*/*py
Replace spaces with underscores in filenames because spaces cause problems in file handling

This stops github from recognising that files are the same, which is a pity! This will need to be resolved to merge hpc CLOVER with new features in main branch

Bash commands:

rename -s ' scripts' '_scripts' Scripts/*/

for file in Scripts/*/*py;
do
   sed "s| scripts|_scripts|" $file > temp; mv temp $file;
done
…s those specified in location)

# Set up scripts to allow specification of inputs with kwargs (overrides those specified in location)

bash commands used:

for name in grid location finance GHG device optimisation energy_system scenario diesel
do
	cat > ${name}_line.py << EOF
        # Replace input values with keywords if specified
        if kwargs.get('${name}_inputs'):
            for i in kwargs.get('${name}_inputs'):
                if not i[0] in self.${name}_inputs.index:
                    print("Couldn't find entry",i[0],"in ${name}_inputs. Perhaps it's misspelt in kwargs? Printing list of possible variables and exiting.")
                    print(self.${name}_inputs.index)
                    exit(1)
            self.${name}_inputs[i[0]] = i[1]
EOF

	for file in Scripts/*/*py
	do
		sed "/        self.${name}_inputs = pd.read_csv/r ${name}_line.py" $file > tmp
		mv tmp $file
	done

rm ${name}_line.py

done
Relating to specific projects - I think unlikely to be harmful, but may not be useful
Accomodate revised file structuring (keep Jobs in separate dir and repository from main CLOVER files
Reduced number of devices for example run
Clean up and improve comments
New hpc users don't seem to have a work directory of the same format
Directory structure changed in 2018
Changed file format to remove \r newlines which were causing problems
- Moved launch file & added it's new home to the path in .bash_profile (readme instructions, could also add to setup file)
- Corrected time between renewables ninja requests in Load_Solar_Bhinjpur.py
Hoping this will encourage these necessary folders to be recognised in upload & download of repo
Added indication of which kwargs can/must be specified per script (some way towards Ben's suggested safer way of specifying arguments)

Removed some of my obsolete options (quick battery recharge, efficient diesel - introduced in the earlier "miscellaneous" commit):

Left a few other additions in (which were also introduced in the earlier "miscellaneous" commit) :
- In Optimisation.py: Additional metrics around storage performance reported: LCSE, Cumulative storage cost
- In Load.py: optional arguments for different approaches to load generation
- In Optimisation.py: optional argument to dump spare battery capacity at the end of a simulation period
@sheridanfew
Copy link
Author

Not sure what the issue with the Load script is... Perhaps more info here when logged in as Phil?

@BenWinchester
Copy link
Collaborator

Not sure what the issue with the Load script is... Perhaps more info here when logged in as Phil?

Hi Sheridan, did you merge in the latest changes from the master branch? I think Phil can see what the merge conflicts are when he's logged in maybe 😄 .

Copy link
Collaborator

@BenWinchester BenWinchester left a comment

Choose a reason for hiding this comment

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

I added a few comments 😄 . There are lots of .csv files cropping up everywhere, do these need to be committed? There are also the solar files, which I thought needed to be generated by the user? I don't know whether it makes sense to commit these is all?

@@ -0,0 +1,14 @@
# coding: utf-8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a biggie, but adding a shebang to the start of the file will tell the terminal that it should execute the code in bash. It probably won't ever cause an issue, but is good practice for avoiding potential bash vs term etc. issues 😄
https://linuxize.com/post/bash-shebang/

@@ -0,0 +1,184 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

For instance, this is the Shebang in this file which tells the terminal interpreter to use /bin/sh to execute the script.

@@ -0,0 +1,36 @@
# coding: utf-8
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can also add shebangs to python files, though this isn't necessary provided they're executed with python my_script.py 😄
(For python, it would be /usr/bin/python3.7 or something similar. Using which python3 or which python in the command line/terminal will point you to the python installation that's used by default.)


# Import scripts
print('Importing Scripts...')
exec(open("Scripts/Load_scripts/Load.py").read())
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the os module is imported, it might be good to use its inbuilt functionality to avoid issues when switching between Windows/Mac/Linux file systems in terms of how the / vs \ is used as the path separator.

os.path.join("Scripts", "Load_scripts", "Load.py")

will return "Scripts/Load_scripts/Load.py" on a linux system and "Scripts\Load_scripts\Load.py" if someone is using an operating system that uses backslashes 😄

@@ -19,7 +19,7 @@
import scipy

class Conversion():
def __init__(self):
def __init__(self,**kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like **kwargs is used in the Conversion class at all..? Using the location: str=None syntax for specifying keyword arguments picks up on things like this 😄

def get_connections_expenditure(self,households,year=0):
'''
Function:
Calculates cost of connecting households to the system
Inputs:
households DataFrame of households from Energy_System().simulation(...)
households DataFrame of households from Energy_System(**self.kwargs).simulation(...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unneeded change? 😛

# kwargs can include scenario_inputs, location_inputs, device_inputs, diesel_inputs, grid_inputs, finance_inputs, GHG_inputs (passed on to other scripts to override inputs in location files)
self.kwargs = kwargs
self.location = kwargs.get('location')
self.CLOVER_filepath = '.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, the os.getcwd() would work well. Hard-coding in something like . shouldn't really be needed 😄

filename Name of .csv file to be saved as (defaults to timestamp)
Outputs:
Simulation saved to .csv file
"""
if not os.path.exists(self.simulation_storage):
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this block, the os module provides a method that covers this case without needing an if statement:

os.makedirs(self.simulation_storage, exist_ok=True)

This will make the folder if it doesn't exist already, and won't throw an error, or overwrite the folder, if it already exists.

@@ -359,7 +386,7 @@ def get_storage_profile(self, start_year = 0, end_year = 4, PV_size = 10, **opti

# Initialise power generation, including degradation of PV
PV_generation = PV_size * pd.DataFrame(self.get_PV_generation()[start_hour:end_hour].values
* Solar().solar_degradation()[0:(end_hour-start_hour)].values)
* Solar(**self.kwargs).solar_degradation()[0:(end_hour-start_hour)].values)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not best sticking kwargs on the class as it's unclear what's stored in it and what's being passed through 🐱

@@ -0,0 +1,31 @@
wd=$(pwd)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shebang 😱

@BenWinchester BenWinchester linked an issue Jul 1, 2021 that may be closed by this pull request
@BenWinchester BenWinchester added enhancement New feature or request duplicate This issue or pull request already exists labels Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add HPC functionality to CLOVER
2 participants