Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write last time node of each cycle rather than EOC to database #1090

Merged
merged 17 commits into from
Jul 16, 2024

Conversation

onufer
Copy link
Member

@onufer onufer commented Jan 16, 2023

Description

This PR Removes the interactEOC database write and forces the write at the last time step instead. The figure below shows the before and after. State points at red lines are written to database. State points with a circle around them are all at the same exact time in the reactor life (within the armi model). The bottom half is the new direction that this PR follows.

Note: After this PR, most databases will have 1 more time node, such that there is at least one accessible node for every time in the Reactor.

image

Close #1100


Checklist

  • This PR has only one purpose or idea.
  • Tests have been added/updated to verify that the new/changed code works.
  • The release notes (location doc/release/0.X.rst) are up-to-date with any bug fixes or new features.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in setup.py.

@john-science john-science added the feature request Smaller user request label Jan 16, 2023
@albeanth
Copy link
Member

albeanth commented Jan 17, 2023

@onufer what does this PR do? I can infer what the changes are to loadOperator in armi/bookkeeping/db/__init__.py, but it's not clear what's going on with the changes in bookkeeping/db/databaseInterface.py.

EDIT:
Thank you for the PR description!

@john-science john-science marked this pull request as draft January 17, 2023 23:23
@john-science john-science changed the title write EOC and EOL explictly straw man Writing EOC and EOL explicitly to the DB Jan 17, 2023
@onufer
Copy link
Member Author

onufer commented Jan 18, 2023

closed in favor of #1100

@albeanth albeanth closed this Jan 18, 2023
@opotowsky opotowsky deleted the writeLastNode branch October 10, 2023 18:39
@john-science john-science restored the writeLastNode branch June 13, 2024 15:40
@john-science john-science reopened this Jun 13, 2024
@john-science
Copy link
Member

I can't wait!

We will want unit tests on this new feature at some point. Should be straight forward.

@john-science
Copy link
Member

Can we add a release note under "API Changes"?

armi/doc/release/0.3.rst

Lines 20 to 22 in b3986c1

API Changes
-----------
#. Replacing the concrete material with a better reference. (`PR#1717 <https://github.com/terrapower/armi/pull/1717>`_)

I would like it to say that we added a new time node, and slightly modified the meaning of one that existed. (Even if the change is small, this is the place where we tell people it happened.)

@john-science
Copy link
Member

@onufer Can you re-run ruff? It looks like the linting CI is broken.

@john-science
Copy link
Member

When this PR is ready for final review, you can take it out of "Draft".

@onufer onufer marked this pull request as ready for review July 15, 2024 21:34
@john-science john-science self-requested a review July 15, 2024 22:22
@onufer onufer changed the title Writing EOC and EOL explicitly to the DB Write last time node rather than EOC to database Jul 15, 2024
@onufer onufer changed the title Write last time node rather than EOC to database Write last time node of each cycle rather than EOC to database Jul 15, 2024
Copy link
Member

@keckler keckler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mark. This change was less painful to make than I expected.

armi/bookkeeping/db/databaseInterface.py Outdated Show resolved Hide resolved
armi/bookkeeping/db/databaseInterface.py Show resolved Hide resolved
armi/bookkeeping/mainInterface.py Outdated Show resolved Hide resolved
doc/release/0.3.rst Outdated Show resolved Hide resolved
@keckler
Copy link
Member

keckler commented Jul 16, 2024

Sorry, last question -- Is there really not any documentation that needs to be updated with this change?

@john-science
Copy link
Member

Sorry, last question -- Is there really not any documentation that needs to be updated with this change?

Well, as I noted somewhere (here or in the ticket?) there is no place in the docs where we explain the time steps in the DB. I imagine that should go some where in doc/user/input.rst, but I'm not immediately sure where.

It's certainly a strange omission, but I think that needs to be fixed outside this PR. It does need to be fixed though.

@john-science john-science merged commit f04b365 into main Jul 16, 2024
19 checks passed
@john-science john-science deleted the writeLastNode branch July 16, 2024 16:16
@onufer onufer mentioned this pull request Jul 16, 2024
7 tasks
opotowsky pushed a commit that referenced this pull request Jul 17, 2024
Bug fixes for PR #1090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make EOC available in the DB
5 participants