Skip to content

Fix bad units in IDD for swimming pool misc power input#4637

Merged
Myoldmopar merged 1 commit intodevelopfrom
972110-SwimmingPool-FixIDD
Dec 18, 2014
Merged

Fix bad units in IDD for swimming pool misc power input#4637
Myoldmopar merged 1 commit intodevelopfrom
972110-SwimmingPool-FixIDD

Conversation

@mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Dec 18, 2014

@Myoldmopar @kbenne Would be good to get this in for the next iteration build.

Myoldmopar added a commit that referenced this pull request Dec 18, 2014
Fix bad units in IDD for swimming pool misc power input
@Myoldmopar Myoldmopar merged commit 5b34fa7 into develop Dec 18, 2014
@mjwitte
Copy link
Contributor Author

mjwitte commented Dec 18, 2014

@Myoldmopar Thanks. So, should I post a new issue to suggest that we add an IDD validator to the CI tests? Or a pivotal ticket? IDF Editor has code in it, but I think there are some other scripts out there already (perhaps the one that @mmAtTs used for the curve referencing cleanup) that could be the basis for checking that all of the \codes are valid, and all of the \object-list referencing is valid.
@JasonGlazer

@Myoldmopar
Copy link
Member

I'd say make a Pivotal ticket. I have an idd parser that would be very easy to use here, but it is in VB.Net, so it would be WIndows only...but that's all we need anyway. At one point, I had started translating it to python, so it might be a very light workload to get this validation working well. Feel free to assign it to me.

@kbenne
Copy link
Contributor

kbenne commented Dec 18, 2014

Let me just say I am not in favor of VB or Python validation being bolted on here. The idea of the project is bring your compiler and cmake and press the compile button. Those should be the dependencies. The alternative starts to look like a Frankenstein of Legos, Erector Set, and Lincoln Logs. It is bad enough that Python is required to do regression tests. Sure you can say, but this is an isolated, advanced use case. I don't agree.

This use case of idd validation seems especially touchy, because those are facilities that are in one way or another captured in the EnergyPlus code itself. Don't compete with that or reinvent it. If you want to use idd validation outside of the engine, then factor that component out of EnergyPlus and use that. I know it is more work, but that is exactly the kind of modular (and unit testable) EnergyPlus code that you want anyway.

See my comments to @refaqtor (I don't know how to link to specific comments) about reusing idf parsing that already exists. I think the same thing applies here. #4636

@Myoldmopar
Copy link
Member

(@kbenne, if you look at the top of the comment itself, there is a time-note, such as "Commented 2 minutes ago", the time text is the link to the specific comment)

@kbenne
Copy link
Contributor

kbenne commented Dec 18, 2014

Very nice. #4636 (comment)

@mjwitte
Copy link
Contributor Author

mjwitte commented Dec 18, 2014

@kbenne I understand the concern of adding more dependencies. The reason I suggested this is because EnergyPlus does not validate or even use many of the \codes. Some of them it does, but many it does not, and for some it's more forgiving. But IDF Editor and presumably other tools do use them and complain when they are not valid. I wasn't intending this to even be part of the build, but rather a utility that runs on the CI just like mathdiff (assuming we're still using that for the regressions - or equivalent). Or we could add more validation in to EnergyPlus, but that may slow down the IDD parsing. I'll open a pivotal ticket and we can discuss further under that.

@Myoldmopar Myoldmopar deleted the 972110-SwimmingPool-FixIDD branch December 19, 2014 18:24
@Myoldmopar Myoldmopar added the Defect Includes code to repair a defect in EnergyPlus label Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Defect Includes code to repair a defect in EnergyPlus

4 participants