Constant for large values in Cyclus/Cycamore #606

bennibbelink · 2024-05-18T23:42:19Z

cyclus/cyclus#1757 updates some archetypes to use a new constant instead of 1e299. This updates cycamore to use the same constants. CI will only pass when this PR and cyclus 1757 are tested together, so please refer to the cyclus PR for test status.

github-actions · 2024-05-18T23:54:26Z

Build Status Report - `aa67f87` - 2024-07-21 21:20:28 +0000

Build `FROM cyclus_20.04_apt/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_apt/cyclus:stable`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_20.04_conda/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_20.04_conda/cyclus:stable`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_apt/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_apt/cyclus:stable`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

Build `FROM cyclus_22.04_conda/cyclus:latest`

Cycamore: Success ✅
Cymetric: Success ✅

Build `FROM cyclus_22.04_conda/cyclus:stable`

Cycamore: Failure ❌
Cymetric: Skipped due to upstream failure ⚠️

gonuke

A question related to how this all fits together in the cyclus ecosystem

gonuke · 2024-05-19T15:15:40Z

src/enrichment.h

-    "range": [0.0, 1e299], \
+    "range": [0.0, "cyclus::cy_large_double"], \


So this has me thinking about where else this metadata is used... If you have an installed build with this branch, can you run cyclus -m and see what it reports as the range? Has the substitution happened at that point?

Other tools may read/parse this metadata without access to cyclus::cy_large_double

The substitution has not happened at that point

Do we want those values to be expanded to their numeric equivalents? I'll see if it can be done given the build process in place

There are two issues here...

on one hand it would be good to have expanded so that other users of this metadata know what it is

if we want modules to be plug-an-play with different installs of Cyclus, maybe we want this to be substituted later??

It's possible we could do both, but performing the substitution before we publish the metadata? That might be a bigger project though!

What if we substitute later and have a section of metadata (or a different cli flag perhaps) that shows users the values for constants in this install of cyclus? That way we don't need to modify the preprocessor, modules are plug-and-play, and users have a way to see what the constants are (as long as we put them in an intuitive place)

Related - how is the substitution made under your current paradigm - when is it substituted and what can it be used for?

Right now cyclus::cy_large_double is a static const double so it is evaluated at runtime and exists in the cyclus C++ namespace (and in the cyclus.lib python library via cython wrapper). It is not evaluated before/by cycpp.py. We could probably #define the value instead so it is substituted by cpp (though I'm still not sure there's a way to substitute it into the #pragma cyclus statement to be evaluated before cycpp.py is run). Does that answer your question?

I'm trying to recall if there is any code on the Cyclus side that uses those values and were the substitution happens to use those values. For example, if the range of a variable is set in these pragmas, I think it results in an XML grammar entry that requires the value to be within that range. Is the substitution done before generating the snippet of grammar so that XML only knows about the numerical value? Or is the substitution happening later?

It's also possible that the value is not actually used anywhere by Cyclus and only exists for other tools that use the metadata.

KFacility, Sink, and Source in cyclus use this value. When calling cyclus -m the variable name is still included in metadata instead of the value. However, when viewing an agent's xml schema I don't see any reference at all. For example - capacity in Source is defined as so:

#pragma cyclus var { \ "doc": "amount of commodity that can be supplied at each time step", \ "uilabel": "Maximum Throughput", \ "uitype": "range", \ "range": [0.0, "cy_large_double"], \ "tooltip": "source capacity" \ } double capacity;

But this is what I see when printing the agent schema via the cyclus cli (is this incomplete?):

root@a52714cb3b92:/cyclus# cyclus --agent-schema :agents:Source <interleave> <element name="commod"> <data type="token"/> </element> <optional> <element name="recipe_name"> <data type="token"/> </element> </optional> <element name="capacity"> <data type="double"/> </element> </interleave>

The section of JSON dedicated to Source when calling cyclus -m is:

":agents:Source": { "all_parents": [ "cyclus::Agent", "cyclus::Facility", "cyclus::Ider", "cyclus::StateWrangler", "cyclus::TimeListener", "cyclus::Trader" ], "doc": "A minimum implementation source facility that provides a commodity with a given capacity", "entity": "facility", "name": "cyclus::Source", "parents": ["cyclus::Facility"], "vars": { "capacity": { "alias": "capacity", "doc": "amount of commodity that can be supplied at each time step", "index": 2, "range": [0.0, "cy_large_double"], "shape": [-1], "tooltip": "source capacity", "type": "double", "uilabel": "Maximum Throughput", "uitype": "range" }, "commod": { "alias": "commod", "doc": "commodity that the source facility supplies", "index": 0, "schematype": "token", "shape": [-1], "tooltip": "source commodity", "type": "std::string", "uilabel": "Commodity", "uitype": "outcommodity" }, "recipe_name": { "alias": "recipe_name", "default": "", "doc": "Recipe name for source facility's commodity.If empty, source supplies material with requested compositions.", "index": 1, "schematype": "token", "shape": [-1], "tooltip": "commodity recipe name", "type": "std::string", "uilabel": "Recipe", "uitype": "recipe" } }

bennibbelink · 2024-06-01T01:22:53Z

With some inspiration from the preprocessor docs, the substitution now takes place during the execution of cycpp.py. There are two sets of variables maintained, one in C++ namespace (in src/cyc_limits.h) and one in Python namespace (in cyclus/system.py). Archetype developers can use #pragma cyclus exec to import the constants into the global python namespace during cycpp.py execution (thus they can be used to define variable metadata). I just got this into a working state, I welcome suggestions for improvement!

CI in this PR will not pass since it is based on the published cyclus docker images - see cyclus/cyclus#1757 for CI status. Still failing on one test but not related to the new constants - need to resolve #611 then we should see CI pass in the cyclus PR

EDIT: CI now passing in cyclus/cyclus#1757

gonuke · 2024-07-18T21:17:18Z

Can you check whether any new large numbers crept in during recent PRs? A rebase and search, perhaps?

gonuke · 2024-07-18T21:20:18Z

I don't understand how this is passing? What cyclus version is it using to get the right values for the constants?

bennibbelink · 2024-07-19T21:04:02Z

Can you check whether any new large numbers crept in during recent PRs? A rebase and search, perhaps?

I will check and make a comment after.

I don't understand how this is passing? What cyclus version is it using to get the right values for the constants?

I'm seeing failures for all 8 builds... remember there are no "soft" failures in Github actions so we must refer to the bot's comment for the correct build statuses (unless I'm misunderstanding)

gonuke · 2024-07-20T13:12:24Z

I guess I already forgot our standard testing regimen - or perhaps more accurately was confused between cyclus and cycamore.

(For my own benefit...) standard CI testing for cyclus/cyclus has hard failures and soft failures for downstream testing. Standard CI testing for cyclus/cycamore has ONLY soft failures because all testing uses some upstream repo whose state may or may not be consistent.

Given that, in this particular case, what should we be reviewing in cycamore - just the code itself and ignoring the tests?

gonuke

A couple of little things here - they aren't critical, but nice to have.

gonuke · 2024-07-20T13:16:33Z

src/sink.h

                      "doc": "When a random distribution is used to determine the " \
-                             "frequency of the request, this is the upper bound. Default 1e299"}
+                             "frequency of the request, this is the upper bound. Default CY_LARGE_INT (1e299)"}


Can we get the actual value of the variable to appear here rather than hardcoding 1e299 in the string? Would this work?

Suggested change

"frequency of the request, this is the upper bound. Default CY_LARGE_INT (1e299)"}

"frequency of the request, this is the upper bound. Default CY_LARGE_INT (" + str(CY_LARGE_INT) + ")"}

I think this is ultimately processed in python, so not sure how the value of this metadata is passed along.

Good catch, this is now replaced at build time

src/storage.cc

bennibbelink · 2024-07-21T20:17:54Z

(For my own benefit...) standard CI testing for cyclus/cyclus has hard failures and soft failures for downstream testing. Standard CI testing for cyclus/cycamore has ONLY soft failures because all testing uses some upstream repo whose state may or may not be consistent.

Correct.

Given that, in this particular case, what should we be reviewing in cycamore - just the code itself and ignoring the tests?

Yes, except since we have an associated PR in cyclus/cyclus we can use those CI tests as a reference. That's why I have the cyclus PR temporarily referencing this PR - we can see that CI passes when the two branches are in sync with each other

…cyclus_large

bennibbelink · 2024-07-21T22:05:59Z

Can you check whether any new large numbers crept in during recent PRs? A rebase and search, perhaps?

Just went through and replaced a handful of instances of hardcoded constants. It looks like they were arbitrary small/large values, but someone else should go through and confirm. I also went through cyclus and replaced some hardcoded values.

gonuke · 2024-07-26T20:02:11Z

I'm ready to merge this - let me know if we need to coordinate @bennibbelink - or should I go ahead with this?

bennibbelink · 2024-07-27T12:16:17Z

I've changed the ref in cyclus/cyclus back to cyclus/cycamore, so they are both ready to be merged. I don't think we need to coordinate, regardless of how we do this there will be an intermediate state where the projects are out of sync. With that being said... maybe it makes sense to merge cyclus first then see CI pass on cycamore just since thats the typical workflow (of introducing features to cyclus then catching up downstream)?

That may contradict a comment I made earlier about the appropriate merge procedure but I think this makes sense, what do you think?

bennibbelink · 2024-07-28T18:52:43Z

Re-ran CI and this is passing on the :latest image, should be good to go.

gonuke reviewed May 19, 2024

View reviewed changes

bennibbelink force-pushed the cyclus_large branch from 39510fb to 29d0271 Compare June 1, 2024 00:17

bennibbelink marked this pull request as draft June 1, 2024 00:22

bennibbelink mentioned this pull request Jun 13, 2024

Constant for large values cyclus/cyclus#1757

Merged

1 task

bennibbelink requested a review from gonuke June 13, 2024 01:29

bennibbelink marked this pull request as ready for review July 18, 2024 20:42

gonuke requested changes Jul 20, 2024

View reviewed changes

bennibbelink and others added 9 commits July 21, 2024 16:20

convert instances of e299 to cyclus limit constants

828657e

remove requirement of agnostic "python" in environment

797ae81

use new constants

88f9eff

stash pop and resolve conflicts

1057949

specify cyclus:: namespace for cy_large_int

7bd93ae

update CHANGELOG

a1cc8ff

use #pragma cyclus exec to use constants

4c1d311

revert some debugging changes that were committed

e601fd3

replace CY_LARGE_INT in doc

f7da6a3

bennibbelink force-pushed the cyclus_large branch from a92a151 to f7da6a3 Compare July 21, 2024 20:22

bennibbelink added 4 commits July 21, 2024 16:50

replace other instances of large doubles and near-zero values

96bb027

Merge branch 'main' into cyclus_large

bd7ddde

add back tu_name_ arg, it was accidentally deleted

a66905c

Merge branch 'cyclus_large' of github.com:bennibbelink/cycamore into …

aa67f87

…cyclus_large

gonuke approved these changes Jul 26, 2024

View reviewed changes

gonuke merged commit aea4bb5 into cyclus:main Jul 28, 2024
10 checks passed

bennibbelink mentioned this pull request Aug 1, 2024

Add instructions for using constants in archetype development cyclus/cyclus.github.com#380

Closed

bennibbelink mentioned this pull request Aug 18, 2024

Prevent excess resource database lines from being generated when nothing happens cyclus/cyclus#1775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constant for large values in Cyclus/Cycamore #606

Constant for large values in Cyclus/Cycamore #606

bennibbelink commented May 18, 2024

github-actions bot commented May 18, 2024 •

edited

Loading

gonuke left a comment

gonuke May 19, 2024

bennibbelink May 20, 2024

gonuke May 20, 2024

bennibbelink May 20, 2024

gonuke May 20, 2024

bennibbelink May 20, 2024

gonuke May 23, 2024

bennibbelink May 23, 2024 •

edited

Loading

bennibbelink commented Jun 1, 2024 •

edited

Loading

gonuke commented Jul 18, 2024

gonuke commented Jul 18, 2024

bennibbelink commented Jul 19, 2024

gonuke commented Jul 20, 2024

gonuke left a comment

gonuke Jul 20, 2024

bennibbelink Jul 21, 2024

bennibbelink commented Jul 21, 2024

bennibbelink commented Jul 21, 2024

gonuke commented Jul 26, 2024

bennibbelink commented Jul 27, 2024 •

edited

Loading

bennibbelink commented Jul 28, 2024

		"range": [0.0, 1e299], \
		"range": [0.0, "cyclus::cy_large_double"], \

	"frequency of the request, this is the upper bound. Default CY_LARGE_INT (1e299)"}
	"frequency of the request, this is the upper bound. Default CY_LARGE_INT (" + str(CY_LARGE_INT) + ")"}

Constant for large values in Cyclus/Cycamore #606

Constant for large values in Cyclus/Cycamore #606

Conversation

bennibbelink commented May 18, 2024

github-actions bot commented May 18, 2024 • edited Loading

Build Status Report - aa67f87 - 2024-07-21 21:20:28 +0000

Build FROM cyclus_20.04_apt/cyclus:latest

Build FROM cyclus_20.04_apt/cyclus:stable

Build FROM cyclus_20.04_conda/cyclus:latest

Build FROM cyclus_20.04_conda/cyclus:stable

Build FROM cyclus_22.04_apt/cyclus:latest

Build FROM cyclus_22.04_apt/cyclus:stable

Build FROM cyclus_22.04_conda/cyclus:latest

Build FROM cyclus_22.04_conda/cyclus:stable

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

bennibbelink May 23, 2024 • edited Loading

Choose a reason for hiding this comment

bennibbelink commented Jun 1, 2024 • edited Loading

gonuke commented Jul 18, 2024

gonuke commented Jul 18, 2024

bennibbelink commented Jul 19, 2024

gonuke commented Jul 20, 2024

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

bennibbelink commented Jul 21, 2024

bennibbelink commented Jul 21, 2024

gonuke commented Jul 26, 2024

bennibbelink commented Jul 27, 2024 • edited Loading

bennibbelink commented Jul 28, 2024

github-actions bot commented May 18, 2024 •

edited

Loading

Build Status Report - `aa67f87` - 2024-07-21 21:20:28 +0000

Build `FROM cyclus_20.04_apt/cyclus:latest`

Build `FROM cyclus_20.04_apt/cyclus:stable`

Build `FROM cyclus_20.04_conda/cyclus:latest`

Build `FROM cyclus_20.04_conda/cyclus:stable`

Build `FROM cyclus_22.04_apt/cyclus:latest`

Build `FROM cyclus_22.04_apt/cyclus:stable`

Build `FROM cyclus_22.04_conda/cyclus:latest`

Build `FROM cyclus_22.04_conda/cyclus:stable`

bennibbelink May 23, 2024 •

edited

Loading

bennibbelink commented Jun 1, 2024 •

edited

Loading

bennibbelink commented Jul 27, 2024 •

edited

Loading