-
Notifications
You must be signed in to change notification settings - Fork 393
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
Refactor internal source inputs #8442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems really great. The only CI issue I see so far is unused object warnings, which are probably just because the objects aren't marked as being used. Do you have additional work to do on this branch or is it ready for testing?
ShowContinueError(state, "The temperature calculation after layer parameter has been set to one less than the number of layers."); | ||
thisConstruct.TempAfterLayer = thisConstruct.TotLayers - 1; | ||
} | ||
std::string construction_name{UtilityRoutines::MakeUPPERCase(fields.at("construction_name"))}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need an inputProcessor->markObjectAsUsed(CurrentModuleObject, instance.key());
to avoid the ERR diffs popping up on this branch.
@Myoldmopar I'll fix that |
@jasondegraw This has no milestone. Is it intended for 9.5? |
@mjwitte I'd like it to be for 9.5, but would need some help on the transition work. Any chance you could help me with that? If not we can put it down for E+ future. |
@jasondegraw Sure I can do the transition code. Go ahead and get this up to date and ready for review. |
@mjwitte I think I've got the documentation all fixed up and hopefully the unused object errors too. |
CI is looking good here, is it just the transition rules remaining? |
@Myoldmopar Yes, after I fixed that last unit test failure (newly merged in test that was using the old object), as far as I know the only thing left is transition. |
Transition added and develop merged in. @Myoldmopar Tag! Be sure to test the transition on a few files. |
used for control is the surface listed above in the field for Surface | ||
Name. If the user enters a group of surfaces for that input, the | ||
\emph{first} surface in the radiant group is the surface for control | ||
purposes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a minor conflict because this got modified in the #8530 PR. I think I got it resolved properly.
NwNumArgs = CurArgs - 5 | ||
OutArgs(1) = InArgs(1) | ||
OutArgs(2:NwNumArgs) = InArgs(7:CurArgs) | ||
CALL WriteOutIDFLines(DifLfn,'Construction',NwNumArgs,OutArgs,NwFldNames,NwFldUnits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was about to comment on how you also wrote out the object accidentally but I see you leveraged the written
variable above to avoid it. Looks good.
Pulled in develop locally to resolve conflicts, no major issues, built OK and unit tests run fine. Code looks good. CI looks good. I ran transition and the output diff looks good and the transitioned file runs fine (warning in VentilatedSlab.idf, but probably not related to this). I will push up my changes and merge this thing. Thanks @jasondegraw and @mjwitte |
Pull request overview
This PR implements the first part of the addition of a moisture source to the HAMT feature. That new feature adds a moisture internal source object, and this refactoring sets it up so that the inputs for the thermal problem and the moisture problem are more similar. Previously , internal heat sources were specified using the
Construction:InternalSource
object. After this PR is merged, the same input is done with a regularConstruction
and a newConstructionProperty:InternalHeatSource
object:becomes
A later PR will add a
ConstructionProperty:InternalMoistureSource
object. This will need transition and documentation, I need to reinstall MikTeX before I can get the docs updated. Barring any errors in changes to the test files, there should only be diffs about the IDD changes.Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.