New feature for DeployInst #529

abachma2 · 2022-01-26T17:18:30Z

This PR adds a feature to the DeployInst: The facilities deployed by this institution are now registered with a GrowthRegion and how these facilities contribute to meeting any specified demand in the region.

I have added new tests to the DeployInst unit tests and a new input file to the regression tests to test how the DeployInst, GrowthRegion, and ManagerInst all interact with each other. Changes in the unit tests include some changes to the ManagerInst tests to prevent duplicate function definitions that threw errors when building.

All unit tests for DeployInst, GrowthRegion, and ManagerInst pass (although there were other errors in the unit tests, see #528), and the integration test passes.

…er-inst

…l cyclus::ManagerInst instead.

opotowsky

GREAT WORK! This is a valuable contribution on its own (likely for you and other users), but is also shockingly timely with my personal needs.

I left a few comments about, maybe some require discussion so I'll keep an eye out for replies.

There is one not-small change we need to discuss: the integration tests. I'd like some input from you, @gonuke, @nuclearkatie, and/or anyone else on if this should be tested in test_regression.py or if we should make a new test file, test_integration.py. Because this is upgrading DeployInst behavior, I could see an argument for regression, but the original intention was an integration test ensuring DeployInst and ManagerInst play correctly together within GrowthRegion.

Thanks so much for your hard work on this!

P.S. This was edited to delete some confusion over where the regression test input files live. There is a dir (tests/input/) that has input files that aren't used for these tests (maybe they were once upon a time). The input files for the tests actually come from input/.

src/deploy_inst.h

opotowsky · 2022-01-26T21:22:45Z

src/manager_inst_tests.cc

@@ -1,10 +1,10 @@
 #include "manager_inst_tests.h"

 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-TestProducer::TestProducer(cyclus::Context* ctx) : cyclus::Facility(ctx) {}
+//TestProducer::TestProducer(cyclus::Context* ctx) : cyclus::Facility(ctx) {}


Is there a reason this line and line 7 are commented out?

I ran cycamore_unit_tests with and without these commented out and both times it ran 195 tests... I didn't do a clean build in between so maybe that's why?

When these lines are included (not commented out) I got errors when building cycamore because of duplicate definitions. Removing this and line 7 removes the duplication. You most likely didn't run into this issue because it was happening on the build and not with the unit tests. I will remove the commented out lines.

I see they were added in deploy_inst_tests.cc (along with the corresponding class definition in deploy_inst_tests.h) so that is where the TestProducer class duplication is coming from, since it was also defined in manager_inst_tests.h.

I'm not sure what these lines do/don't do. I'm guessing it needs to test DeployInst as a commodity producer, which is needed, and further, it is likely needed for both ManagerInst and DeployInst.

I tried deleting the duplicated definition from deploy_inst_tests.h, and including manager_inst_tests.h in deploy_inst_tests.cc. Not sure that's good practice, though, but it didn't work either way. I'll play around a little more.

All of the tests in manager_insts_tests.cc are being run without these lines. My guess is that the initialization of the function in deploy_insts_tests.cc is still valid for this one.

input/growth/deploy_and_manager_insts.xml

opotowsky · 2022-01-26T22:03:38Z

input/growth/deploy_and_manager_insts.xml

+    <spec> <lib>cycamore</lib><name>Sink</name> </spec>
+  </archetypes>
+
+  <commodity>


I'm not sure this block is even needed for this simple simulation. At least, upon skimming the resulting DBs with and without it, I didn't see a difference. We can get someone more knowledgable to weigh in.

I felt like it was good practice to include a sink in the simulation. Also, this way it matches more with the source_sink_linear.xml file also in that directory.

Oh I meant the commodity block with the priority, not the sink. I should have done a better job w that comment :-)

(this is a comment of the lowest priority, btw)

That was just a force of habit. If you really think it needs to be removed I can do that.

It depends if we prefer the input file to be a good example or to be bare bones. I was reviewing from the bare bone perspective, which given the remaining questions in Issue #530 may or may not be valid.

This is definitely not a necessary block, but I would NOT require its remove for this to merge.

This reverts commit 3a98763.

This reverts commit ad57d01, reversing changes made to 43ac611.

This reverts commit 1fca401.

src/deploy_inst_tests.cc

opotowsky

Changes are looking good! I still have questions about the TestProducer class duplication issue and what exactly is happening, which hopefully @gonuke or @nuclearkatie can help with.

In my initial review I had raised a question about whether the "integration test" belongs in test_regression.py or not....I'm fine with it there but would like others to also weigh in.

gonuke

Wow @abachma2 - this is a great deep dive into the dusty corners of Cyclus! Because you've done that dive, I think you can better answer any questions that arise. I'd be happy to jump on a call to discuss anything.

Is testing exhaustive enough?
Can a deploy inst still be used without a growth region? I guess that it can since there exist tests that do this. I'm just not sure how that is handled...

gonuke · 2022-02-06T19:18:09Z

src/deploy_inst.h

-      " list of prototypes for" \
+      " decommissioned at the end of their lifetime.  Deployed and" \
+      " decommissioned agents are registered and unregistered with a" \
+      " region. The user specifies a list of prototypes for" \


do we need to specify that it's a Manager region?

I'm not sure what you mean by Manager region.

Just that it's important to document what kind of region these agents are registered with. I think that's important to note in the documentation, but maybe it's more generic??

I think this is where she previously had "growth region" and I asked that "growth" be removed because in theory it could work within another region?

That's fair - it probably shouldn't be limited to a growth region, specifically, but - on the other hand - GrowthRegion is the only region that implements the toolkit to be useful for registration....

@opotowsky is correct, I removed the word growth to try and make it as region agnostic as possible. I thought that the growth region was the only one currently implemented that uses registration, but this wording would be flexible for if a new one was developed.

gonuke · 2022-02-06T19:48:37Z

src/manager_inst.h

- private:
  /// register a child
  void Register_(cyclus::Agent* agent);

  /// unregister a child
  void Unregister_(cyclus::Agent* agent);
-
+
+  protected:


Why this change? I don't see anything that requires it in the other changes

I don't remember why I did that. I can undo that change. Would you also recommend making these functions private in src/deploy_inst.h as well?

its good practice for them to be private unless we need them to be otherwise

gonuke · 2022-02-06T19:52:41Z

src/manager_inst_tests.cc

@@ -1,11 +1,5 @@
 #include "manager_inst_tests.h"

-// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-TestProducer::TestProducer(cyclus::Context* ctx) : cyclus::Facility(ctx) {}


This line is probably necessary, but we may need to discuss some of the code structure because it is not potentially shared between two institutions.

In can probably be defined inline in the single place where the TestProducer is declared.

gonuke · 2022-02-06T19:54:14Z

src/deploy_inst_tests.h

+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+class TestProducer
+    : public cyclus::Facility,
+      public cyclus::toolkit::CommodityProducer {
+ public:
+  TestProducer(cyclus::Context* ctx);
+  ~TestProducer();
+
+  cyclus::Agent* Clone() {
+    TestProducer* m = new TestProducer(context());
+    m->InitFrom(this);
+    return m;
+  }
+
+  void InitFrom(TestProducer* m) {
+    cyclus::Facility::InitFrom(m);
+  }
+
+  void InitInv(cyclus::Inventories& inv) {}
+
+  cyclus::Inventories SnapshotInv() { return cyclus::Inventories(); }
+
+  void Tock() {}
+  void Tick() {}
+};


This might need to go into a separate header file to be included in both the manager_inst and the deploy_inst tests.

gonuke · 2022-02-06T19:56:28Z

src/deploy_inst_tests.cc

+TestProducer::TestProducer(cyclus::Context* ctx) : cyclus::Facility(ctx) {}
+
+// - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+TestProducer::~TestProducer() {}
+


It looks like these were moved from the manager_inst - this is probably not the best place for them.

I did move these lines from src/manager_inst_tests.cc. As far as I can tell having these defined in both places breaks the linking to the unit tests when building. I understand the desire to define them somewhere else that does not feel that they are exclusive to just the one set of tests. Any recommendations on that?

They definitely need to be only defined in one place. If they are in two places, they get compiled twice and then the linker finds two copies when it tries to combine them.

I think you can create a file test_producer.h that contains the definition of TestProducer from deploy_inst_test.h and defines these two functions inline in that header/class definition. That header can then be included in both of the institution header files.

gonuke · 2022-02-06T20:00:57Z

input/growth/deploy_and_manager_insts.xml

+    <spec> <lib>cycamore</lib><name>Sink</name> </spec>
+  </archetypes>
+
+  <commodity>


This is definitely not a necessary block, but I would NOT require its remove for this to merge.

abachma2 · 2022-02-07T21:01:54Z

To answer both of your questions @gonuke, I have added a new integration test to test the function of the DeployInst with a NullRegion. This does pass. The DeployInst does work with other regions. I think the test suite is exhaustive, unless there is another region I am not aware of that should have its integration with the DeployInst tested.

abachma2 · 2022-02-09T22:59:03Z

@gonuke suggested moving some of the duplicate definitions between src/deploy_inst_test.h and src/manager_inst_tests.h into a separate header that could be read in by both, but that was not possible without creating a dummy prototype within cycamore that was blank. So the duplications remain, until a better solution is found.

Also, with the use of TestProducer by both src/deploy_insts_tests.cc and src/manager_insts_tests/cc, it seems that all of the information from the unit tests are stored while the tests are being run so this is not a major issue for now, since all of the unit tests and regression tests are running and passing. We should revisit this in the future though.

abachma2 and others added 25 commits March 26, 2021 13:40

new files and small name changes to create deploymanagerinst

8fc752c

fixed typo

31839c4

missed instance, fixed typo

2c69856

Merge branch 'master' of github.com:cyclus/cycamore into deploy-manag…

7653f29

…er-inst

added test file

3a53159

missed a few instances near the end

1a1483d

inherit from managerinst, installs, tests fail

064b850

makes explicit calls to functions in cyclus::Institution actually cal…

3e2d6b8

…l cyclus::ManagerInst instead.

pulled in BuildNotify from ManagerInst, tests still fail

8ce30e9

copied functions from ManagerInst, tests pass, works with toy problem

dc1e951

added tests from ManagerInst, new tests don't pass

d574667

changed to TEST_F and all tests pass now

2e93d6e

cleaned up extra lines, added functionality to comments

6078305

added sample test for corner case test

882b853

filled in test with cyclus mock sim

ca24a31

changed mocksim definition

8fe92de

added definition for prototypes in mocksim

bb8e75f

removed config for growthregion

bc52e9c

changed config name ack

797e8fe

added additional MockSims, all separate sims, need to combine

416ba1f

added commodity and capacity to dummyprotos

031ab59

Removed DeployManagerInst, added feature to DeployInst

63ac68a

added input for integration tests

a001062

moved functions around to make unit tests build and pass

43ac611

Merge branch 'master' into deploy-manager-inst

ad57d01

abachma2 requested review from nuclearkatie and opotowsky January 26, 2022 17:18

abachma2 self-assigned this Jan 26, 2022

abachma2 added Status: 5-Review labels Jan 26, 2022

opotowsky requested changes Jan 26, 2022

View reviewed changes

opotowsky mentioned this pull request Jan 26, 2022

Unit tests not passing for Storage archetype #528

Closed

abachma2 added 4 commits January 27, 2022 07:58

changes suggested from @opotowsky

3a98763

Revert "changes suggested from @opotowsky"

b8a2925

This reverts commit 3a98763.

Revert "Merge branch 'master' into deploy-manager-inst"

1fca401

This reverts commit ad57d01, reversing changes made to 43ac611.

changes requested from @opotowsky

7fa2a09

abachma2 marked this pull request as draft January 27, 2022 14:22

abachma2 added 4 commits January 27, 2022 09:04

added commodity recipe, needed for tests

ed47d55

added new test for integration of deploy and manager insts

df920e1

Revert "Revert "Merge branch 'master' into deploy-manager-inst""

41383f8

This reverts commit 1fca401.

Merge branch 'master-changes' into deploy-manager-inst

34c0517

abachma2 marked this pull request as ready for review January 27, 2022 16:22

opotowsky reviewed Jan 27, 2022

View reviewed changes

src/deploy_inst_tests.cc Outdated Show resolved Hide resolved

abachma2 added 2 commits January 27, 2022 10:49

modified doc sting of TestGrowth2

f327056

removed extra includes, moved the one non-duplicate

3262056

opotowsky reviewed Jan 27, 2022

View reviewed changes

gonuke requested changes Feb 6, 2022

View reviewed changes

abachma2 added 2 commits February 7, 2022 14:59

added new test for deployinst + nullregion

50d1f66

input file for new test

6384312

abachma2 added 2 commits February 9, 2022 16:48

made register and unregister private

f8e3f6e

put TestProducer back into manager_inst_tests.cc

43137a2

gonuke approved these changes Feb 10, 2022

View reviewed changes

gonuke requested a review from opotowsky February 10, 2022 02:13

opotowsky mentioned this pull request Feb 10, 2022

TestProducer duplication #532

Open

opotowsky approved these changes Feb 10, 2022

View reviewed changes

opotowsky merged commit c7506f1 into cyclus:master Feb 10, 2022

abachma2 mentioned this pull request Feb 11, 2022

Made new inputs for HALEU transition analysis arfc/transition-scenarios#123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New feature for DeployInst #529

New feature for DeployInst #529

abachma2 commented Jan 26, 2022

opotowsky left a comment •

edited

Loading

opotowsky Jan 26, 2022

opotowsky Jan 26, 2022

abachma2 Jan 27, 2022

opotowsky Jan 27, 2022

abachma2 Jan 27, 2022

opotowsky Jan 26, 2022

abachma2 Jan 27, 2022

opotowsky Jan 27, 2022

opotowsky Jan 27, 2022

abachma2 Jan 27, 2022

opotowsky Jan 27, 2022

gonuke Feb 6, 2022

opotowsky left a comment

gonuke left a comment

gonuke Feb 6, 2022

abachma2 Feb 7, 2022

gonuke Feb 7, 2022

opotowsky Feb 7, 2022

gonuke Feb 7, 2022

abachma2 Feb 7, 2022

gonuke Feb 6, 2022

abachma2 Feb 7, 2022

gonuke Feb 7, 2022

gonuke Feb 6, 2022

gonuke Feb 6, 2022

gonuke Feb 6, 2022

abachma2 Feb 7, 2022

gonuke Feb 7, 2022

gonuke Feb 7, 2022

gonuke Feb 6, 2022

abachma2 commented Feb 7, 2022

abachma2 commented Feb 9, 2022

New feature for DeployInst #529

New feature for DeployInst #529

Conversation

abachma2 commented Jan 26, 2022

opotowsky left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opotowsky left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abachma2 commented Feb 7, 2022

abachma2 commented Feb 9, 2022

opotowsky left a comment •

edited

Loading