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

Make EOC available in the DB #1100

Closed
onufer opened this issue Jan 18, 2023 · 7 comments · Fixed by #1090
Closed

Make EOC available in the DB #1100

onufer opened this issue Jan 18, 2023 · 7 comments · Fixed by #1090
Assignees
Labels
feature request Smaller user request

Comments

@onufer
Copy link
Member

onufer commented Jan 18, 2023

Here is a table for an example simulation with three 30-day depletion steps and a 10 day outage. Providing this as an initial strawman... see discussion below for why it is this way currently:

Node Time (days) Written to DB
Beginning of Cycle(BOC) 0 no (same time as 0,0)
0,0 0 x
0,1 30 x
0,2 60 x
0,3 90 NO! (90 days isn't anywhere to be found)
End of Cycle(EOC) 100 x (as the 0,3 state point, but its not, its 10 days past..)

The first question is, Why is there no write at 0,3, but instead there is a write at EOC? This is so we can do restart runs from the end of a cycle. if you loaded from the data at 0,3 instead of EOC you and started the next cycle you would miss the decay step and the problem you would have run would diverge from the original by the decay step since currently when you load from 0,2 it jumps right into interact BOC for the next cycle (cycle 1 (1,0) ) rather than EOC from 0,2.

What's bad about this? If you want to retrieve the isotope distribution from 0,3 its not retrievable. only EOC is retrievable. Snapshot runs are much more interested in the 0,3 data than the EOC data. So the fundamental problem is the restarts want the EOC data but snapshots want the 0,3 data. We could potentially solve this by writing the 0,3 data instead of EOC and having restart run interact EOC when its restarting from the last node. I'm sure there are other solutions out there too such as redefining when the outage occurs... maybe it could be the first thing in a cycle and that might solve the misMatch, but its not super clear to me.

One solution that I discourage is writing both 0,3 AND EOC since this will increase the database size by a factor of ~1.25 (5/4) and haver very little valuable additional info. (was investigated here #1090 )

@keckler
Copy link
Member

keckler commented Jan 18, 2023

I think it would be useful to clarify how the issue of writing only EOC isotopics to the DB is related to having time advancement be on the operator itself.

To me, those seem like related but different issues. From your description I don't quite understand how you think that the operator issue will resolve the DB issue.

You might have something in mind that I'm just not understanding...

@onufer
Copy link
Member Author

onufer commented Jan 18, 2023

@keckler, you are right ,its really 2 issues.

  1. Operator should advance time... both @john-science and @ntouran agree.
  2. Once we decide this we should figure out a better way of doing it, which most of this ticket talks about it. if we are moving time advancement to the framework, it should be done right.

I tried to clarify above

@keckler
Copy link
Member

keckler commented Mar 2, 2023

Another issue with this, aside from the isotopics at the last time node being irretrievable, is that the results written to the last DB node are misleading.

For instance, the last DB node will look just like any other, so it will have keff. But that keff doesn't actually correspond to the isotopics that are written to the same DB node, because that last node would have extra decay time in there in comparison to when the value of keff was actually calculated. So if a user were to load up the isotopics from the DB and recalculate keff, they'd get a (potentially very) different answer.

I would imagine that to be very confusing to a user who doesn't understand this quirk.

One thing which might offer a path forward is the fact that, for a given assembly, EOC for cycle n is the same point in time as BOC for cycle n+1. Because of this, our current DBs actually have a lot of duplicated isotopic vectors between EOC of one cycle and BOC of the next. The reactor might be in a different configuration at n+1 BOC due to shuffling, but the isotopics in each given assembly are the same.

So if we were to deplete the assemblies for the end-of-cycle-decay at the EOC node for cycle n and then write those depleted isotopics to a DB point for node 0 of cycle n+1, then the isotopic vectors for each assembly would properly correspond to the time at that node, and we would avoid overwriting the isotopics at the last node of cycle n without hugely increasing the size of the DB. And since the BOC node doesn't do any depletion calcs anyways, we won't overwrite the isotopics from that side either.

I think this could work even with shuffling taking place at BOC of cycle n+1 because the isotopic vectors live on the blocks, so the shuffling operations when you get to the interactBOC() hook would remove the isotopics associated with assemblies that are shuffled out of the core. If you wanted to keep the isotopics from those removed assemblies, you'd have to switch on trackAssems.

This would require the restart logic to be slightly modified to correctly handle restarts at different time nodes.

Not sure this is the BEST solution, but it seems like some sort of viable option.

@john-science john-science self-assigned this Sep 21, 2023
@john-science
Copy link
Member

Okay, we just had a meeting about this ticket.

A big take-away from the meeting is (in the example above) we want to write the "0,3" time step along with the "EOC" time step to the Database.

The presumed best path forward with this would be to just add "more stuff" to the Database, not remove the "EOC" that is there now. (And don't include both if they are exactly the same time.)

@drewj-usnctech
Copy link
Contributor

Chiming in because @keckler pointed me here from #1475

We would benefit from a standardized thing that sets Reactor.p.time

@john-science
Copy link
Member

john-science commented Dec 22, 2023

We would benefit from a standardized thing that sets Reactor.p.time

That's an interesting point.

@john-science john-science changed the title Operator should be responsible for advancing time Make EOC available in the DB Feb 26, 2024
@keckler
Copy link
Member

keckler commented Jul 15, 2024

This ticket is linked to be closed by #1090 , but it actually was documenting two problems:

  1. Operator should advance time
  2. Writing EOC/EOL to the DB

Only (2) will be closed by the linked PR.

Can we get another ticket opened to track (1)?

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 a pull request may close this issue.

4 participants