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

Migrate combustion and EPW-reader from Hive.Core to Hive.IO #692

Merged
merged 16 commits into from
Dec 22, 2021

Conversation

maxenceryan
Copy link
Contributor

@maxenceryan maxenceryan commented Dec 16, 2021

Issues

Closes #686

Description

Migrate the epw-reader from ghpy to Hive.IO

  • EPW-reader now reads the EPW class given by the Hive.Environment instead of re-parsing the EPW file
  • ins and outs are the same except
    • formatting of City, Country is slightly different
  • only main template and a new environment GH test file were changed

TODO:

  • @christophwaibel in many gh example files, a simple path is given to the python EPW reader, which now no longer works. Should I change all the testfiles to reflect the proposed set up (epw-path -> environment -> hive inputs -> env distributor -> epw reader instead of just epw-path -> epw-reader)? Or is it better to keep an option for simply giving the path? We could also move the parsing to the epw-reader instead of the environment component.

Checklist

Max Ryan added 2 commits December 15, 2021 22:17
…eader, include icons in resources, create epw distributor, update env distributor
@maxenceryan maxenceryan self-assigned this Dec 16, 2021
@christophwaibel
Copy link
Contributor

hey @maxenceryan , I suggest to keep an option for a simple workflow without needing to go through the whole environment->envDistributor pipeline. Yes, I think it makes sense to move the parsing to the EpwReader component. Please also note our naming convention of using a Prefix Gh(...) for all grasshopper components

Comment on lines 70 to 83
DA.SetData(0, Latitude);
DA.SetData(1, Longitude);
DA.SetData(2, City, Country);
DA.SetData(3, GHI);
DA.SetData(4, DNI);
DA.SetData(5, DHI);
DA.SetData(6, DryBulb);
DA.SetData(7, DewPoint);
DA.SetData(8, RH);
DA.SetData(9, GHIMonthly);
DA.SetData(10, DryBulbMonthly);
DA.SetData(11, RHMonthly);
DA.SetData(12, AmbientTemperatureCarrier);
DA.SetData(13, Timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

so this is computationally very slow, because grasshopper turns everything into GH_Numbers. Could you only output an epw-object by default, and these separate outputs only get written if a boolean is set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually the wrong cs file, i forgot to delete it... you'll see the other one in Hive.IO. but your comment still applies. So would the idea be that this distributor only functions when the bool input is true? Should I pass the EPW object to other components, such as sia380 and solar stuff instead of the gh numbers?

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be excellent! As much as possible, we should avoid dumping out gh numbers. It exceeds a bit the original scope of this issue, but if you can already implement that (pasing only the epw object instead of separate lists of gh numbers), that would be marvelous

Copy link
Contributor

Choose a reason for hiding this comment

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

And , yes , only dump out those lists if a bool is set to true. From a novel grasshopper user perspective , that would be useful to access information from an epw

Comment on lines 24 to 25
pManager.AddTextParameter("Path", "Path", "Path of the epw file", GH_ParamAccess.item);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add `pManager.AddBooleanParameter("write GH_Numbers", (...), "additionally to the epw-object, weather file data is output explicitly separately", ...)

@maxenceryan maxenceryan changed the title 686 migrate combustion Migrate EPW-reader from Hive.Core to Hive.IO Dec 16, 2021
@maxenceryan
Copy link
Contributor Author

an issue with having parsing only in the EPW distributor is that the GhEnvironment component actually needs EPW already parsed in order to set a couple of the default energy potentials, which are passed along at the EnvDistributor.

l69-71 in Environment.SetDefaultEnergyPotentials()

            this.EnergyPotentials[6] = new Air(this.Horizon, null, null, null, this.EpwData.DryBulbTemperature);
            this.EnergyPotentials[6].Name = "DryBulbTemperature";
            this.EnergyPotentials[7] = new Radiation(this.Horizon, this.EpwData.GHI);

We couuuld just say either plug in an Hive.IO.EPW class object OR the filepath into the EPW Distributor component, with priority given to the EPW object if not null.

@christophwaibel
Copy link
Contributor

an issue with having parsing only in the EPW distributor is that the GhEnvironment component actually needs EPW already parsed in order to set a couple of the default energy potentials, which are passed along at the EnvDistributor.

l69-71 in Environment.SetDefaultEnergyPotentials()

            this.EnergyPotentials[6] = new Air(this.Horizon, null, null, null, this.EpwData.DryBulbTemperature);
            this.EnergyPotentials[6].Name = "DryBulbTemperature";
            this.EnergyPotentials[7] = new Radiation(this.Horizon, this.EpwData.GHI);

We couuuld just say either plug in an Hive.IO.EPW class object OR the filepath into the EPW Distributor component, with priority given to the EPW object if not null.

right. then let the GhEnvironment already parse the epw and, as you said, the GhDistEpw either reads in a path (and parses the epw itself), or reads in an epw object and doesn't need to parse itself anymore.

the boolean, I might have explained it incorrectly. So the parsing should happen either way, but just dumping out the DataLists should be optional

@maxenceryan this is what I meant. does it make sense?
@maxenceryan
Copy link
Contributor Author

sounds good, your comment and commit make sense to me. Hopefully I can wrap this up by monday

@maxenceryan maxenceryan marked this pull request as ready for review December 20, 2021 09:28
@maxenceryan
Copy link
Contributor Author

@christophwaibel ok so I finished up the EPW reader stuff as per the comments. You can see how it compares to the python solution in GrasshopperExamples\Testing\Hive_Environment_Test.gh. The assert code for comparing C# and python component is a tad messy... but it would throw an error if they didnt match.

image

You can also look in GrasshopperExamples\Testing\Hive_Distributor_Test.gh to see how it would look like from now on.

image
image

A few things;

  • I still need to update the EPW component in the other GH files, so a commit will be incoming.
  • Once you are done with your review, it'd be nice if you could:
    • comment out the python epw-reader building in build.cmd
    • delete the python component and assert madness in GrasshopperExamples\Testing\Hive_Environment_Test.gh.
  • Finally, i had already migrated the simple combustion components, so they are still there. As you said, we don't need them anymore, so should I for sure delete them or save them somewhere?

Thanks!

@christophwaibel
Copy link
Contributor

christophwaibel commented Dec 21, 2021

A few things;

  • I still need to update the EPW component in the other GH files, so a commit will be incoming.

should I wait for another commit? otherwise, you can merge this PR, @maxenceryan

  • comment out the python epw-reader building in build.cmd

can't do yet, because there is a ghpython component in the main template that relies on following lines from epw_reader.py:

clr.AddReferenceToFileAndPath(os.path.join(path, "Libraries\Hive", "Hive.IO.gha"))
import Hive.IO.EnergySystems as ensys

I'll make a separate issue about it.

  • delete the python component and assert madness in GrasshopperExamples\Testing\Hive_Environment_Test.gh.

done

  • Finally, i had already migrated the simple combustion components, so they are still there. As you said, we don't need them anymore, so should I for sure delete them or save them somewhere?

as discussed before in the sprint: let's keep them for now

Comment on lines 29 to 31
{
pManager.AddNumberParameter("Latitude", "Latitude", "Latitude in deg", GH_ParamAccess.item);
pManager.AddNumberParameter("Longitude", "Longitude", "Longitude in deg", GH_ParamAccess.item);
Copy link
Contributor

Choose a reason for hiding this comment

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

first output: pManager.AddCustom...?Parameter("EPW Hive object", ...)?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah... I mean AddGenericObjectParameter

Comment on lines 9 to 11
{
public class EpwReader : GH_Component
{
Copy link
Contributor

Choose a reason for hiding this comment

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

GhEpwReader

Comment on lines 67 to 69

// TODO

Copy link
Contributor

Choose a reason for hiding this comment

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

we could just call Environment/epw.cs here and leave the parsing script where it is

Comment on lines 70 to 83
DA.SetData(0, Latitude);
DA.SetData(1, Longitude);
DA.SetData(2, City, Country);
DA.SetData(3, GHI);
DA.SetData(4, DNI);
DA.SetData(5, DHI);
DA.SetData(6, DryBulb);
DA.SetData(7, DewPoint);
DA.SetData(8, RH);
DA.SetData(9, GHIMonthly);
DA.SetData(10, DryBulbMonthly);
DA.SetData(11, RHMonthly);
DA.SetData(12, AmbientTemperatureCarrier);
DA.SetData(13, Timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

That would be excellent! As much as possible, we should avoid dumping out gh numbers. It exceeds a bit the original scope of this issue, but if you can already implement that (pasing only the epw object instead of separate lists of gh numbers), that would be marvelous

Comment on lines 70 to 83
DA.SetData(0, Latitude);
DA.SetData(1, Longitude);
DA.SetData(2, City, Country);
DA.SetData(3, GHI);
DA.SetData(4, DNI);
DA.SetData(5, DHI);
DA.SetData(6, DryBulb);
DA.SetData(7, DewPoint);
DA.SetData(8, RH);
DA.SetData(9, GHIMonthly);
DA.SetData(10, DryBulbMonthly);
DA.SetData(11, RHMonthly);
DA.SetData(12, AmbientTemperatureCarrier);
DA.SetData(13, Timezone);
Copy link
Contributor

Choose a reason for hiding this comment

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

And , yes , only dump out those lists if a bool is set to true. From a novel grasshopper user perspective , that would be useful to access information from an epw

@ChrisZenhub
Copy link

ChrisZenhub commented Dec 21, 2021

@maxenceryan when merging with the current master, you can keep the EaCS3_E04_Hive_Template.gh from this PR and overwrite master. But @mmatache25 did some changes to the EnergySystems Hizard, so keep an eye on what you merge from Hive.IO.csproj.

Also, I'm not sure if this PR already merged the updated PV / BIPV catalogues

@maxenceryan
Copy link
Contributor Author

ok so merging now. Last thing I did:

  • removed Hive.Core.combustion from build.cmd as it is now in Hive.IO. NSI script also does not install it and removes the ghpy if it was there from a previous version
  • kept epw-reader in build.cmd and as part of installer for now
  • fixed the compiling script for BIPV, it is now compiled to JSON with PV and ST (although will not be accessible for the user until adding BIPV as separate technology to the Energy Systems Hizard #677)

@maxenceryan maxenceryan changed the title Migrate EPW-reader from Hive.Core to Hive.IO Migrate combustion and EPW-reader from Hive.Core to Hive.IO Dec 22, 2021
@maxenceryan maxenceryan merged commit ce83574 into master Dec 22, 2021
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.

Migrate Hive.Core.epw-reader to C#
3 participants