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

Update pythonocc to enable Mac ARM support #3265

Merged
merged 30 commits into from Oct 13, 2023
Merged

Conversation

reyery
Copy link
Member

@reyery reyery commented Nov 15, 2022

Could potential fix #3120

@ShiZhongming
Copy link
Collaborator

We paused merging this PR for now until @reyery figure it out what to do with it.

@reyery
Copy link
Member Author

reyery commented Apr 28, 2023

Not sure if this can be merged since the results for the thermal networks is very different due to changes to wntr

@jimenofonseca maybe you can take a look at the differences and see if it makes sense. I attached the outputs in the zip file
Archive.7z.zip

@reyery
Copy link
Member Author

reyery commented Apr 28, 2023

@MatNif maybe you have some ideas as well. Apparently one of the changes in wntr is that it is using EPANET version 2.00.12 vs 2.2.0, not sure if that matters.

Pressure losses of edges seems to differ by 10^-3

https://wntr.readthedocs.io/en/latest/whatsnew.html#v0-3-0-november-2-2020

@reyery
Copy link
Member Author

reyery commented Apr 28, 2023

seems like one of the differences has something to do with reservoirs, probably due to the "headloss" differences from the simulation results

image

@ShiZhongming ShiZhongming requested a review from MatNif June 8, 2023 08:57
@MatNif
Copy link
Collaborator

MatNif commented Jun 22, 2023

When I try to start up the dashboard from a Windows PC on this branch I am running into a bit of an issue already. It looks like the libpysal.io library is not installed by default. I'm not sure if the warnings-handling in utilities/dbf.py that was there before the changes made in this branch was meant to handle these errors.

Screenshot 2023-06-22 112652

@MatNif
Copy link
Collaborator

MatNif commented Jun 22, 2023

Then, as I run the code of the network-layout script, I run into the following error:

Screenshot 2023-06-22 123510

which might have something to do with the fact that the 'nearest'-method is not available for all versions of the geopandas library:

Screenshot 2023-06-22 123532

@reyery
Copy link
Member Author

reyery commented Sep 11, 2023

I have tried recompiling wntr for the version that we are using (v0.2.3), to avoid the difference in results for the latest version. Everything seems to be working now, and there should not be differences when running it anymore.

@MatNif the EPANET version used in wntr would still be v2.00.12 vs v2.2.0, not sure what the differences are but I guess we can investigate this next time. So no further actions are required from your end.

@ShiZhongming It now works for Apple Silicon Macs natively, it should be a little faster now as well, since there is no need for a translation layer anymore. I have updated a lot of the python dependencies as well, so testing it would be much more complicated, as you would need to create a new conda environment from the new environment file.

First run a workflow using the current environment that you have, in the master branch.

Then checkout to this PR branch, run the following commands in the CEA git repo folder:

mamba env create -n cea-arm -f environment.yml
conda activate cea-arm
pip install -e .

Run the same workflow using this new environment. Compare if the results are the same (especially for thermal network results)

Let me know if you encounter any issues. Thanks!

@reyery reyery requested review from ShiZhongming and removed request for MatNif September 11, 2023 06:16
@ShiZhongming
Copy link
Collaborator

@reyery Thanks for the PR again.
I have just tested it as instructed using two scenarios: one in Zurich and one in Singapore.

For thermal-network, for both scenarios using master and this PR, the results were identical.
For solar-potentials, in the Singapore scenario, the results were identical, while in the Zurich scenario, the results are mostly slightly different. I have attached the two .csv here.

Zurich-PR:
PV_total_buildings.csv

Zurich-Master:
PV_total_buildings.csv

@reyery
Copy link
Member Author

reyery commented Sep 13, 2023

Interestingly there are differences in the area of PV, but I don't think it is that significant. Would it be possible to try a larger scenario and see if affects more buildings.

@ShiZhongming
Copy link
Collaborator

Hi @reyery
I have just tested this PR using a much larger scenario (>200 buildings) in Zurich. The results are identical.
I think we can merge this! :D

@reyery reyery merged commit d59d224 into master Oct 13, 2023
3 checks passed
@reyery reyery deleted the mac-arm-native-support branch October 13, 2023 06:10
@ShiZhongming ShiZhongming restored the mac-arm-native-support branch October 13, 2023 14:50
@reyery reyery deleted the mac-arm-native-support branch April 24, 2024 15:38
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.

Apple silicon machines require Rosetta to be installed to run CEA
3 participants