Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON #7036

jmarrec · 2018-10-26T16:06:38Z

Pull request overview

Fix #6720 duplicate name json

Issue was a problem in doj/alphanum.hpp when you have leading zeros. Basically it computes a number for the left side, and another for right side, and compares that. But that means "01" and "001" end up being the same, so it reports them as being equal. Adding a clause where when diff is zero, you also check the length of the left and right sides, and potentially return that.

Added unit tests, that fail without the fix (see https://github.com/jmarrec/EnergyPlus/tree/Do_Not_Publish/6720_UnitTestOnly) and work with it.

Rationale for leading zeroes

I had to made a choice for the case where you have leading zeroes as I couldn't really find any information online: I assume that if you have two strings, "n_001" and "n_1", I assume you want "n_001" < "n_1" (behavior is easy to change if you disagree)

Work Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
- Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
- Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
- NewFeature: This pull request includes code to add a new feature to EnergyPlus
- Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
- DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

Using std::less in json.hpp for object_comparator makes t

Myoldmopar · 2018-11-09T03:03:57Z

@jmarrec could you pull develop into this branch and see if the test failures were due to a CI thing or if this branch actually breaks everything?

jmarrec · 2018-11-09T10:57:47Z

@Myoldmopar done.

Even though I push to my remote, any maintainer has write access to it so feel free to do that kind of stuff.

If that's easier I could push to NREL/EnergyPlus in the future instead if you prefer (but that increases the attention needed to delete merge branches though in order to not clutter everything).

Myoldmopar · 2018-11-11T01:18:31Z

@jmarrec so this is causing lots of issues. Our CI bots are failing (panicking on Linux...) due to bad allocations as they run out of memory. Something has gone very awry with this. I need to close this PR so that our CI machines don't continue to try to run.

@mjwitte this is the cause of the memory issue I saw which caused me to ramp down the number of processors on the Linux machine. I'll be ramping it back up now...

Myoldmopar · 2018-11-11T01:19:06Z

Oh, I kinda left that open-ended. @jmarrec please continue to investigate this, but again, I'm closing this PR so our CI machines won't continue trying to build it.

…20_DuplicateNameJSON

…l" this way

jmarrec · 2018-11-12T16:43:22Z

@Myoldmopar I caught a tiny mistake in the alphanum stuff (my bad, copy paste error).
I built on Ubuntu and didn't get any problems running the test (but then again, neither did I before). I also built on Windows just now and was able to run the tests as well.

Could we try the linux CI bots again?

Myoldmopar · 2018-11-13T12:32:47Z

CI is slightly happier with this branch now 🙃 👍

I'll take a look at this one today.

mbadams5

The changes are appropriate in the alphanum library and there are good unit tests for this bug.

mbadams5 · 2018-11-13T17:41:10Z

third_party/doj/alphanum.hpp


 /* $Header: /code/doj/alphanum.hpp,v 1.3 2008/01/28 23:06:47 doj Exp $ */

+
+// MODIFIED: Julien Marrec, EffiBEM, on 2018-10-26 to avoids "n_001" being recognized as the same as "n_01"


Typically we haven't been adding attribution/bug issue comments like this in new E+ code and especially in third party libraries.

mbadams5 · 2018-11-13T17:42:34Z

tst/EnergyPlus/unit/InputProcessor.unit.cc

+    };
+
+    // This all works just fine
+    test("Standard insulation");


Interesting use of lambda's in the unit test. Seems like it might be overkill for this unit test, but I can think of other unit tests where this approach may come in handy for sure.

Myoldmopar

@jmarrec I'm going to push a comment-only commit and merge this in. Thanks!

Myoldmopar · 2018-11-13T17:40:43Z

tst/EnergyPlus/unit/InputProcessor.unit.cc

+    // Add the first material to it
+    root[obj_name][name1] = mat1;
+
+    auto test = [=](std::string search_name) {


@jmarrec it seems odd to put a lambda in here as opposed to making another function, but it's OK with me.

Myoldmopar · 2018-11-13T17:53:45Z

third_party/doj/alphanum.hpp


 /* $Header: /code/doj/alphanum.hpp,v 1.3 2008/01/28 23:06:47 doj Exp $ */

+
+// MODIFIED: Julien Marrec, EffiBEM, on 2018-10-26 to avoids "n_001" being recognized as the same as "n_01"


@jmarrec it's important to track our changes to third_party libraries, but I don't like comments as the mechanism. These can easily get overridden when we bring in a new version of the library. I'm going to transform this comment into the start of a new file at third_party/PATCHES.md where we'll keep track of all the changes we make to third_party libraries.

jmarrec · 2018-11-13T21:00:54Z

I’m still learning the E+ coding style so please continue to flag unusual stuff like this (and please excuse said unusual stuff, I don’t do it on purpose!).

As for the use of lambda here, it’s a short code block that wouldn’t be used by any other test (but I hate repeating myself) so I thought a lambda made sense here. (And I guess I could have cleaned it up a bit though)

jmarrec added 7 commits October 26, 2018 15:06

Unit test for NREL#6720

ad97829

Lower level test for NREL#6720

b920b4b

Another test for NREL#6720, this time without "."

8af358b

Ok, nailed the location of the bug in DOJ / ALphanum

d2635fe

Using std::less in json.hpp for object_comparator makes t

Fix issue in alphanum

4a143d1

Ok, really fix it this time

2d530e4

Extend unit tests

bce316c

jmarrec mentioned this pull request Oct 26, 2018

JSON validation duplicate name problem #6720

Closed

3 tasks

jmarrec added the Defect Includes code to repair a defect in EnergyPlus label Oct 26, 2018

jmarrec changed the title ~~Fix #6720 duplicate name json~~ Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON Oct 26, 2018

Merge branch 'develop' into PR_opened/Fix_6720_DuplicateNameJSON

82cbfdd

Myoldmopar closed this Nov 11, 2018

jmarrec added 4 commits November 12, 2018 13:55

Merge remote-tracking branch 'upstream/develop' into PR_opened/Fix_67…

25c4698

…20_DuplicateNameJSON

Auto indent so I can compare with original

71e6367

Caught a mistake

4f75906

Change default behavior for leading zeroes, I think it's more "natura…

df5e324

…l" this way

jmarrec reopened this Nov 12, 2018

mbadams5 approved these changes Nov 13, 2018

View reviewed changes

Myoldmopar approved these changes Nov 13, 2018

View reviewed changes

Clean out comment from code and add new PATCHES.md file

e2629aa

Myoldmopar merged commit 022bd03 into NREL:develop Nov 13, 2018

jmarrec deleted the PR_opened/Fix_6720_DuplicateNameJSON branch November 20, 2018 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON #7036

Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON #7036

jmarrec commented Oct 26, 2018 •

edited

Loading

Myoldmopar commented Nov 9, 2018

jmarrec commented Nov 9, 2018

Myoldmopar commented Nov 11, 2018

Myoldmopar commented Nov 11, 2018

jmarrec commented Nov 12, 2018

Myoldmopar commented Nov 13, 2018

mbadams5 left a comment

mbadams5 Nov 13, 2018

mbadams5 Nov 13, 2018

Myoldmopar left a comment

Myoldmopar Nov 13, 2018

Myoldmopar Nov 13, 2018

jmarrec commented Nov 13, 2018 •

edited

Loading


		/* $Header: /code/doj/alphanum.hpp,v 1.3 2008/01/28 23:06:47 doj Exp $ */


		// MODIFIED: Julien Marrec, EffiBEM, on 2018-10-26 to avoids "n_001" being recognized as the same as "n_01"

Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON #7036

Fix #6720 - Objects with different names will trigger a "Duplicate Name" Error in JSON #7036

Conversation

jmarrec commented Oct 26, 2018 • edited Loading

Pull request overview

Rationale for leading zeroes

Work Checklist

Review Checklist

Myoldmopar commented Nov 9, 2018

jmarrec commented Nov 9, 2018

Myoldmopar commented Nov 11, 2018

Myoldmopar commented Nov 11, 2018

jmarrec commented Nov 12, 2018

Myoldmopar commented Nov 13, 2018

mbadams5 left a comment

Choose a reason for hiding this comment

mbadams5 Nov 13, 2018

Choose a reason for hiding this comment

mbadams5 Nov 13, 2018

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar Nov 13, 2018

Choose a reason for hiding this comment

Myoldmopar Nov 13, 2018

Choose a reason for hiding this comment

jmarrec commented Nov 13, 2018 • edited Loading

jmarrec commented Oct 26, 2018 •

edited

Loading

jmarrec commented Nov 13, 2018 •

edited

Loading