Fix floorArea method for Space returning unsorted surfaces #5466
Conversation
src/model/Space.cpp
Outdated
| // get all surfaces, sort so results are repeatable | ||
| std::vector<Surface> surfaces = getObject<ModelObject>().getModelObjectSources<Surface>(Surface::iddObjectType()); | ||
| std::sort(surfaces.begin(), surfaces.end(), IdfObjectNameLess()); |
There was a problem hiding this comment.
Is there a significant performance hit by putting the sort here? If so, we can just sort from within the floorPrint and floorArea methods.
There was a problem hiding this comment.
There could be a lot of surfaces in a model, but hard to imagine that there would that many for a single space.
There was a problem hiding this comment.
Like floorPrint which already sorts surfaces, update floorArea to also sort surfaces. There should be less of a performance hit risk having the sort downstream of the parent surfaces method.
src/model/Space.cpp
Outdated
| boost::optional<double> z; | ||
| std::vector<Surface> floors; | ||
| for (const Surface& surface : surfaces) { | ||
| for (const Surface& surface : this->surfaces()) { |
There was a problem hiding this comment.
I can see how floorPrint could be affected by order of surfaces, since it's doing geometry things with it, but the original issue says it's floorArea that was affected, and as I said at #5465 (comment) I don't see how that's possible.
|
CI Results for f3b8970:
|
| // get all surfaces, sort so results are repeatable | ||
| std::vector<Surface> surfaces = this->surfaces(); | ||
| std::sort(surfaces.begin(), surfaces.end(), IdfObjectNameLess()); | ||
|
|
||
| double result = 0; | ||
| for (const Surface& surface : this->surfaces()) { | ||
| for (const Surface& surface : surfaces) { |
There was a problem hiding this comment.
Ok, so now this is the only change, and it's only done in Space_Impl::floorArea
I was asked
@jmarrec Are you ok with the approach described in #5466 (comment)?
My personnal opinion is that this is still unecessary, and this should be handled by the person trying to compare things.
At least the scope / impact is smaller here.
@joseph-robertson and @shorowit seem to be pretty set on doing it it seems, so I'll leave it up to you to hit the merge button if that's the case.
|
I really need to clean up the number of open PRs, I'm going with the assumption that this is really wanted. Merging. |
Pull request overview
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.