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

Always init default systems #154

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

Conversation

haneslinger
Copy link
Contributor

This PR fixes read_and_create_initial_systems. This function is called in the init of a Facility. It reads the systems from the xml, and if it doesn't have any, provides some defaults.

Now, a close look at function will find a handful of questionable choices, but let's zero down on 3:

  1. Defaults are used in an "all or nothing" fashion. That is, if you have no systems in your xml, you get a default hvac and lighting, ect. But if you have just hvac in the xml, no lighting for you.
  2. systems read from the xml are put in systems_map, default systems are put in class attr. This is confounding. Note no passing integration tests provide there own systems. That's not great
  3. system_mapp allows for multiple hvacs to be defined (its an array), the class attrs only allow one.

In this PR I:

  1. did away with the "all or nothing" fashion
  2. got rid of system_map and always wrote to the class attrs
  3. made the class attrs lists

why do not merge?

the change in how defaults breaks things a little. We are in converstations about how and where defaults should be set.

@haneslinger haneslinger force-pushed the Always-init-default-systems branch from 715f852 to e49dabf Compare February 15, 2024 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants