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

Assign auto instead of 0 to mean unused #1312

Closed
wants to merge 10 commits into from
Closed

Conversation

joseph-robertson
Copy link
Contributor

@joseph-robertson joseph-robertson commented Oct 17, 2024

Pull Request Description

It's confusing to assign 0 to an argument to mean unused. So we'll:

  • still assign 0 if it makes sense as an input (even if unused)
  • assign none when 0 doesn't make sense as an input

Related Pull Requests

[related PRs from different repositories]

Related Issues

Closes #1289.
Closes #1293.

Checklist

Required:

Optional (not all items may apply):

@joseph-robertson joseph-robertson self-assigned this Oct 17, 2024
@joseph-robertson joseph-robertson changed the title Update WH RE values for efficiency options. Update water heater RE values for efficiency options Oct 17, 2024
@joseph-robertson joseph-robertson changed the title Update water heater RE values for efficiency options Assign auto instead of 0 to mean unused Oct 17, 2024
@shorowit
Copy link
Contributor

"We'll leave 0 when: the argument is required"

That's a big gap that will still result in situations where the input is confusing or misleading. In these cases, couldn't we automatically remap "auto" to a valid value here? Or maybe we should reconsider whether some of the BuildResHPXML arguments should be required.

@joseph-robertson
Copy link
Contributor Author

joseph-robertson commented Oct 22, 2024

"We'll leave 0 when: the argument is required"

That's a big gap that will still result in situations where the input is confusing or misleading. In these cases, couldn't we automatically remap "auto" to a valid value here? Or maybe we should reconsider whether some of the BuildResHPXML arguments should be required.

For example, for options with cooling_system_type=none, we set cooling_system_cooling_efficiency=auto. Then at the place you linked to, we set cooling_system_cooling_efficiency=0 (or perhaps to its default value). Something like that?

I suppose it's up to you whether we want to change, e.g., cooling_system_cooling_efficiency to be an optional argument.

@shorowit
Copy link
Contributor

For example, for options with cooling_system_type=none, we set cooling_system_cooling_efficiency=auto. Then at the place you linked to, we set cooling_system_cooling_efficiency=0 (or perhaps to its default value). Something like that?

Yep, exactly. Maybe it would make sense to use 99999 just to ensure that the value isn't being accidentally used, as that would surely cause something to blow up. I don't know if that's a good idea or not.

I suppose it's up to you whether we want to change, e.g., cooling_system_cooling_efficiency to be an optional argument.

I think I'm okay with doing this. Or at least trying. I'm not sure what the disadvantages are exactly. But it must be confusing to users to have to provide a cooling_system_cooling_efficiency value even when they have no cooling system, for example. And we could probably clean up some code in OS-HPXML (like here) if we did this.

@joseph-robertson
Copy link
Contributor Author

Superseded by #1316.

@joseph-robertson joseph-robertson deleted the wh-lookup-fix branch October 25, 2024 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing use of foo=0 for unused variables in options_lookup.tsv ResStock Args consistency with OS-HPXML
2 participants