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

Adds method to process modelica results #646

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Adds method to process modelica results #646

wants to merge 6 commits into from

Conversation

tanushree04
Copy link
Collaborator

@tanushree04 tanushree04 commented Jul 31, 2024

Objective :

  • add uo des cli call and methods to query results from modelica models
  • these results are required for the REOPT GHP LCCA analysis that will be run using the urbanopt SDK
  • this cli call will be run in the following sequence :
    1. create modelica model with GHP
    1. run thermal network to size GHP
    1. run modelica model
    1. query results using this cli call
    1. run reopt GHP LCCA analysis using urbanopt cli

This cli call takes the modelica folder created and run previously as an argument. It then reads the .mat file and queries timeseries results for heating, cooling electricity consumption of heat pumps, ets pump electricity consumption, pump associated with building electricity consumption loop pump electricity consumption and heat pump sizing for the GHP network.

To test :

Use uo_des des-process 'modelica_model_name'

Run poetry run pytest tests poetry run pytest .\tests\geojson_modelica_translator\test_results_ghp.py

Check the "geojson-modelica-translator\tests\geojson_modelica_translator\data\modelica_5\modelica_5.Districts.DistrictEnergySystem_results\modelica_5.Districts.DistrictEnergySystem_result.csv" file creeated with results queried for REopt GHP at 1 hour intervals.

@tanushree04 tanushree04 requested a review from vtnate July 31, 2024 00:30
Copy link
Contributor

@vtnate vtnate left a comment

Choose a reason for hiding this comment

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

Please run pre-commit and address the linting & formatting changes.

Overall it's not clear to me what this command is for. Will you add more context to the PR description (and perhaps a bit to the cli command description) explaining what the output is, why someone would use this command, and how to interpret it?

geojson_modelica_translator/results_ghp.py Outdated Show resolved Hide resolved
@@ -170,28 +171,28 @@ def create_model(sys_param_file: Path, geojson_feature_file: Path, project_path:
@click.option(
"-a",
"--start_time",
default=17280000,
default=15638400,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you changing the defaults here? Did you mean to just pass these arguments to your call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will be changed to run for a year with 15 min timesteps to overcome the openmodelica bug for 1 hour timesteps. We need to report a full year simulations for reopt.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to change the default timestep for all users, we can do that. That would be in --step-size, which was previously at 90 seconds before your change to 1 hour.

To change to running for a full year we'd change the start-time to 0 and the end time to whatever it is in seconds.

management/uo_des.py Outdated Show resolved Hide resolved
:param start_time (int): start time of the simulation (seconds of a year)
:param stop_time (int): stop time of the simulation (seconds of a year)
:param step_size (int): step size of the simulation (seconds)
:param number_of_intervals (int): number of intervals to run the simulation
Copy link
Contributor

Choose a reason for hiding this comment

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

These are valid inputs to the run-model command, aren't they? Why remove them?

management/uo_des.py Outdated Show resolved Hide resolved
"""Post Process the model
\b
Post process results from Modelica project run previously, for GHP LCCA analysis
\b
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space above the \b for it to format as expected when you use it for real.

"""
modelica_path = Path(modelica_project).resolve()
result = ResultsModelica(modelica_path)
result.calculate_results()
Copy link
Contributor

@vtnate vtnate Aug 8, 2024

Choose a reason for hiding this comment

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

This took 2.5 hours to run on my laptop. Is that expected?

print(csv_file_path)

# Check if the CSV file exists
assert csv_file_path.exists(), f"File does not exist at path: {csv_file_path}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This generates a file as expected. I don't know if the contents are appropriate or not. Can you make some additional assertions so we'll know if the file has been populated correctly?

@vtnate vtnate added the enhancement New feature or request label Aug 8, 2024
@tanushree04
Copy link
Collaborator Author

#645

This needs to be incorporated

@vtnate
Copy link
Contributor

vtnate commented Aug 13, 2024

#645

This needs to be incorporated

Ah. That is in #644.

@tanushree04
Copy link
Collaborator Author

pre-co

what is the pre commit command?

@vtnate
Copy link
Contributor

vtnate commented Aug 27, 2024

what is the pre commit command?

poetry run pre-commit run -a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants