Position implementation change #1510

bam241 · 2019-05-20T14:55:06Z

Position change:
The way the Position is working now, each agent have to define their coordinates (all the cycamore have them) and have there own method to Record them (all have the same), what I did, is add the coordinates and the RecordPosition at the Agent level (each cycamore agent still have to get latitude and longitude on their own), each agent calling the RecordPosition at the Cyclus::Agent::EnterNotify() level, avoiding copy/pasting the RecordPosition method over and over...

I also added a RecordPosition(Agent* agent) method in Position, so the Position knows how to register itself in the database. (this add agent and context include in the Position, if we don't like it, it will be as easy to add the RecordPosition in the agent itself...)
I think this last change make sense, as it allows to keep the Agent class fairly simple and will allow each attribute (Position) to know how to Record himself...

This is redondant with #1508 , but @gonuke advised me to do a separate PR for this change.

gonuke · 2019-05-29T16:13:56Z

A better way to streamline this without forcing every agent to have a Position (since that's not in the spirit of the toolkit):

add RecordPosition() to position.h (as already one here)
all agents with a position do the following things
- inherit from Toolkit::Position
- #include a special snippet that adds variables with cyclus pragmas in header (e.g. position.cycpp)
- add Position(latitude,longitude) to the initializer list of their constructor
- call RecordPosition(this) from EnterNotify()

katyhuff · 2019-05-31T22:19:55Z

src/toolkit/position.cycpp

@@ -0,0 +1,19 @@
+#pragma cyclus var { \


Is this file really intended to be part of the PR?

src/toolkit/position.h

tests/toolkit/position_tests.cc

src/toolkit/CMakeLists.txt

bam241 · 2019-06-03T14:07:20Z

@katyhuff thx for your review !
To reply to your comments, the idea here is to include the position.cycpp in the different archetype we develop (#include toolkit/position.cycpp) to avoid having to copy paste over and over:

#pragma cyclus var { \
"default": 0.0, \
"uilabel": "Geographical latitude in degrees as a double", \
"doc": "Latitude of the agent's geographical position. The value should " \
       "be expressed in degrees as a double." \
}
double latitude;

#pragma cyclus var { \
"default": 0.0, \
"uilabel": "Geographical longitude in degrees as a double", \
"doc": "Longitude of the agent's geographical position. The value should " \
       "be expressed in degrees as a double." \
}
double longitude;

so adding position on an archetype, one will only have to:

inherit from Position
#include position.cycpp whithin the class header
call the position constructor for the archetype constructor
call the RecorPosition(this) from the EnterNotify() of the archetype

bam241 · 2019-06-03T14:51:58Z

you can see the related PR draft on cyclus/cycamore#502

bam241 · 2019-06-05T17:50:34Z

@gonuke @katyhuff I believe this is ready for a second review :)

FlanFlanagan · 2019-06-06T22:27:53Z

@bam241 Are we concerned about the cymetric failures? It seems like the tests are returning larger tables than expected.

bam241 · 2019-06-07T04:35:46Z

@FlanFlanagan I did not investigate this yet, but this is just showing that Cyclus has evolve to something that cymetric don't understand.
So I am not concern about it, but we need to PR a fix for symmetric.
the cymetric_master and the cycamore_master are just there as highlight that the impact of a cyclus change on cycamore and cymetric

gonuke

A few requests

tests/toolkit/position_tests.cc

src/toolkit/position.cycpp.h

gonuke · 2019-06-11T16:30:05Z

src/toolkit/position.cycpp.h

@@ -0,0 +1,32 @@
+
+// This includes the required header to add geographic coordinates to a
+// archetypes.


I thinks we need a longer message here that explains how it all works, since it is a brand new concept,

Also, can we use one of the archetypes that’s in cyclus/cyclus to demonstrate?

gonuke · 2019-06-11T16:32:24Z

src/toolkit/position.cycpp.h

+}
+double latitude;
+// required for compilation but not added by the cycpp preprocessor...
+std::vector<int> cycpp_shape_latitude;


What is this for?

this is because cycpp is not run on the file generate by the cpp

see

cyclus/cmake/UseCyclus.cmake

Line 122 in 279a758

SET(ORIG "--pass3-use-orig")

and

cyclus/cli/cycpp.py

Lines 2328 to 2335 in 279a758

parser.add_argument('--pass3-use-pp', action="store_true", default=True,

help=("On pass 3, use the preproccessed version of the "

"original file. This options is mutually exclusive"

"with --pass3-use-orig."), dest="pass3_use_pp")

parser.add_argument('--pass3-use-orig', action="store_false",

help=("On pass 3, use the preproccessed version of the "

"original file. This options is mutually exclusive"

"with --pass3-use-pp."), dest="pass3_use_pp")

so when using this snippet method we miss generated code for the additional included variables...
I try to change the --pass3-use-orig to --pass3-use-pp and cyclus is not compiling :(

I don't know what the best solution is...

cc @gonuke this is a follow-up on the conversation we had this morning...

gonuke · 2019-10-29T22:36:28Z

This needs a news file

gonuke · 2019-11-21T23:54:40Z

will merge with news file!

bam241 · 2019-11-22T00:45:58Z

shall we not wait for the CEP27 to be ready ?

gonuke · 2024-02-07T03:05:30Z

This was paused waiting for CEP27 to be accepted. That did happen in March 2021, so this should be reconsidered.

A similar approach, based on CEP27, would be very valuable for @nuclearkatie's #1634

…ion include

…inate when reccording

…ting

Co-Authored-By: Paul Wilson <paul.wilson@wisc.edu>

bam241 · 2025-04-12T10:20:46Z

@gonuke just rebased this. This does not break any new things :)

coveralls · 2025-04-12T10:36:30Z

Pull Request Test Coverage Report for Build 14419088324

Details

2 of 10 (20.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.003%) to 41.161%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/toolkit/position.cc	2	10	20.0%

Totals
Change from base Build 13100238188:	-0.003%
Covered Lines:	51752
Relevant Lines:	125731

💛 - Coveralls

github-actions · 2025-04-12T10:36:38Z

Downstream Build Status Report - `35cfa7d` - 2025-04-12 11:16:31 +0000

Build `FROM cyclus_20.04_apt/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_apt/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_conda/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_conda/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_apt/cyclus`

Cycamore: Success ✅
Cymetric: Failure ❌

Build `FROM cyclus_22.04_apt/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Failure ❌

Build `FROM cyclus_22.04_conda/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_conda/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_apt/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_apt/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_conda/cyclus`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_24.04_conda/cyclus` --parallel

Cycamore: Success ✅
Cymetric: Success ✅

gonuke · 2025-04-12T15:41:11Z

For some reason, this update will cause cymetric to fail, but only on Ubuntu 22.04 and only those that build with apt and not conda. This usually means that there is some dependency version conflict in the python stack that we don't have enough control over....

Note: such failures are not marked as blocking because when we have intentional changes that break a downstream tool and we have expected updates to that downstream tool, we need to be able to merge under such conditions. This condition is not pathological, but probably deserves a little attention.

bam241 mentioned this pull request May 22, 2019

Adding Metadata, and some other change #1509

Open

2 tasks

katyhuff requested changes May 31, 2019

View reviewed changes

bam241 mentioned this pull request Jun 3, 2019

Update Position cyclus/cycamore#502

Draft

gonuke requested changes Jun 11, 2019

View reviewed changes

bam241 mentioned this pull request Oct 30, 2019

CEP27: toolkit Snippet cyclus/cyclus.github.com#297

Merged

gonuke approved these changes Nov 21, 2019

View reviewed changes

bam241 force-pushed the position branch from 241bcf6 to c8bce6d Compare November 27, 2019 17:31

gonuke added the Feature label Oct 17, 2023

bam241 and others added 12 commits April 12, 2025 12:16

some change in position

8598cae

restauring agent state

b954956

remove position deps in agent.cc, add include snippet, restaure posit…

585a11d

…ion include

add missing include snippet

5587071

fix position.cycpp

78e0102

clang-format

a40c94f

add coordinate in the commun include snippet, fix format of the coord…

822a7c5

…inate when reccording

add some comment in position.cycpp and rename it into position.h.cycpp

6ad6a3e

add missin quote, rename to position.cycpp.h to allow syntax highligh…

407df39

…ting

clang-format

2884035

add missing file, got lost in the renamaing

9588d07

Update src/toolkit/position.cycpp.h

a99fcb4

Co-Authored-By: Paul Wilson <paul.wilson@wisc.edu>

add header initialisation in the snippet

a56077b

bam241 force-pushed the position branch from c8bce6d to a56077b Compare April 12, 2025 10:19

changelog

35cfa7d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Position implementation change #1510

Position implementation change #1510

		@@ -0,0 +1,32 @@

		// This includes the required header to add geographic coordinates to a
		// archetypes.

	parser.add_argument('--pass3-use-pp', action="store_true", default=True,
	help=("On pass 3, use the preproccessed version of the "
	"original file. This options is mutually exclusive"
	"with --pass3-use-orig."), dest="pass3_use_pp")
	parser.add_argument('--pass3-use-orig', action="store_false",
	help=("On pass 3, use the preproccessed version of the "
	"original file. This options is mutually exclusive"
	"with --pass3-use-pp."), dest="pass3_use_pp")

Position implementation change #1510

Are you sure you want to change the base?

Position implementation change #1510

Conversation

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

Pull Request Test Coverage Report for Build 14419088324

Details

💛 - Coveralls

Downstream Build Status Report - 35cfa7d - 2025-04-12 11:16:31 +0000

Build FROM cyclus_20.04_apt/cyclus

Build FROM cyclus_20.04_apt/cyclus --parallel

Build FROM cyclus_20.04_conda/cyclus

Build FROM cyclus_20.04_conda/cyclus --parallel

Build FROM cyclus_22.04_apt/cyclus

Build FROM cyclus_22.04_apt/cyclus --parallel

Build FROM cyclus_22.04_conda/cyclus

Build FROM cyclus_22.04_conda/cyclus --parallel

Build FROM cyclus_24.04_apt/cyclus

Build FROM cyclus_24.04_apt/cyclus --parallel

Build FROM cyclus_24.04_conda/cyclus

Build FROM cyclus_24.04_conda/cyclus --parallel

Downstream Build Status Report - `35cfa7d` - 2025-04-12 11:16:31 +0000

Build `FROM cyclus_20.04_apt/cyclus`

Build `FROM cyclus_20.04_apt/cyclus` --parallel

Build `FROM cyclus_20.04_conda/cyclus`

Build `FROM cyclus_20.04_conda/cyclus` --parallel

Build `FROM cyclus_22.04_apt/cyclus`

Build `FROM cyclus_22.04_apt/cyclus` --parallel

Build `FROM cyclus_22.04_conda/cyclus`

Build `FROM cyclus_22.04_conda/cyclus` --parallel

Build `FROM cyclus_24.04_apt/cyclus`

Build `FROM cyclus_24.04_apt/cyclus` --parallel

Build `FROM cyclus_24.04_conda/cyclus`

Build `FROM cyclus_24.04_conda/cyclus` --parallel