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

Feature: Support Tallies #11

Open
MicahGale opened this issue Jan 22, 2022 · 31 comments
Open

Feature: Support Tallies #11

MicahGale opened this issue Jan 22, 2022 · 31 comments
Assignees
Labels
feature request An issue that improves the user interface. mccafe ☕ Issues that need to be resolved before the MCCAFE MontePy port
Milestone

Comments

@MicahGale
Copy link
Collaborator

MontePy needs to support tally objects.

I think there needs to be a tally object centered around "F" cards. This will then consume other describing data cards to create the hierarchy and description for the tally. This must be linked back to the cells and surfaces in the tallies. @bascandr has requested the ability in the cell object to tell what tallies that cell already has.

I'm a bit hesitant to implement doubly-linking pointers (i.e., tally -> cell, and cell -> tally) due to the complexity updating those links. Perhaps at the problem level the equivalent of a SQL view can be made to support this without the pointers being non-normalized (in Relational database sense), and avoiding delete and update anomalies.

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jan 26, 2022, 14:17

The ability to check which tallies contain a cell will be valuable for MCCAFE development. Even just being able to say [tal for tal in tallies if cell in tal.cells] would be sufficient.

@MicahGale
Copy link
Collaborator Author

Yes that is the direction I was thinking of going. Should the tal.cells be a list or a set? I'm thinking a set.

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jan 27, 2022, 12:55

It should be a set unless you can think of a need for the order to be preserved.

@MicahGale
Copy link
Collaborator Author

I figure before printing set sort it by cell number to be nice and legible.

@MicahGale
Copy link
Collaborator Author

As for how to do the linking backwards to be able to say cell.tallies the pipework for this now exists, and is really easy to setup with generators. See: 3a8c59a.

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jun 7, 2022, 11:02

Full tally support will be really complicated; but adding, removing, and renumbering cells is the low-hanging fruit that will be most useful for my needs.

@MicahGale
Copy link
Collaborator Author

So support the F card at minimum?

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jun 7, 2022, 13:19

Yes

@MicahGale
Copy link
Collaborator Author

What would be the priority for tally modifiers?

  1. FMn
  2. En
    ?

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jun 24, 2022, 13:08

Also FSn - tally segmentation

@MicahGale
Copy link
Collaborator Author

Priority 3?

@MicahGale
Copy link
Collaborator Author

Thoughts about priorities for MOAA needs? @cateal, @fairre?

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Jun 24, 2022, 15:13

Probably priority 1, for ATF. It's used fairly extensively in a lot of past models.

@MicahGale
Copy link
Collaborator Author

In GitLab by @cateal on Jun 24, 2022, 17:22

I am all for FM cards and E cards, which are necessary in MOAA.
I am against FS cards and think they should never be used. Segmenting is not supported by MCNP_TOOLS. Model developers should handle tallies on a cell level and segment by surfaces.

I also agree with what has been said previously that tallies should know (via a set) which cells are handled

@MicahGale
Copy link
Collaborator Author

Hot take much?

@MicahGale
Copy link
Collaborator Author

Personally I've never used FS, and I agree with the FM/E priority. I don't want MCNPy to turn into a linter that yells at users for using a supported feature in MCNP. But I'm not sure if there is merit to de-prioritized FS further. There's a clear use case for it and I don't want to discourage adoption due to a disagreement like this.

@MicahGale
Copy link
Collaborator Author

So new question that I have been thinking about. Obviously tally will have a number. I also want to add tally.type that will be an enum like flux, energy_deposition.... The problem is that MCNP puts the type explicitly in the number. So how should this be handled?

First question should the number include the type? E.g. would F54 have number of 54, 5, or 50?

Secondly should you be able to change the type by setting the number? Like say you want to change F57 to F56?

Thirdly should you be able to change the type and change the number automatically?

@MicahGale
Copy link
Collaborator Author

In GitLab by @cateal on Jul 29, 2022, 12:28

My opinion on this is that the final digit should be treated as a type, then the number is the tally id / 10 (so 5 in your example)

I do like the idea of being able to readily change type, but consider that when changing from 57 to 56, 56 may already be defined.

@MicahGale
Copy link
Collaborator Author

But this will break things like problem.tallies[5] Is that supposed to retrieve f54, f56, f57? I think for that reason the number needs to be fully qualified.

@MicahGale
Copy link
Collaborator Author

I'm kind of split on this. My first thought is that changing the tally type by changing the number should be allowed by raise a warning.

On the other hand say a user wants to change the tally type and tally number, should this really be a two step process? That can become kind of annoying.

@MicahGale
Copy link
Collaborator Author

In GitLab by @tjlaboss on Aug 1, 2022, 12:12

My CAD$0.02 (worth less than USD$0.02):

Each tally type (final digit) is conceptually its own class. Users should not be able to change a tally type by giving it a new number. Rather, it should be a conscious and explicit action.

In other words, my_f4_tally.number = 67 should raise an error, not change the tally from flux to fission heating. A new f7 tally with the same cells, bins, etc. could be simplified with a class method: Tally.fromOtherTally(other, type, cells=None, ...).

In terms of the MCNPy API, problem.tallies[t] should always use the full-number t. Perhaps an OpenMC-like function problem.get_tallies(number=..., type=..., cells=..., surfaces=...) search function could be implemented, or property problem.f7tallies[u] -> t=10*u+7 could be added, but such things aren't necessary.

@MicahGale
Copy link
Collaborator Author

Very good point. You've convinced my on the fact that each tally type ought to be a unique class that is a subclass of Tally. I do like the from_other_tally method. I think for now a tally query function isn't necessary yet. we don't offer a similar thing for cells or surfaces, etc. I think that should be a library wide addition.

@MicahGale
Copy link
Collaborator Author

See #55.

@MicahGale
Copy link
Collaborator Author

This conversation has meandered quite a bit and the infrastructure has greatly improved in MCNPy that I'd like to summarize the design and behavior here based on input:

  1. Create a Tally parent class.

    • This will be a numbered object. The number shall be the full number used by MCNP
    • Will own a Cells instance pointing to Cell objects in the tallies. (Swap out Surfaces for surface tallies)
    • Will implement __contains__ for Cell objects so you can say if cell in tally.
    • Each tally type (f6...) shall be its own subclass
  2. Create Tallies that is a child of NumberedObjectCollection.

  3. Implement cell.tallies that is a generator a la surface.cells.

Support for F, FM and E cards.

I think if you want FS support @tjlaboss you might want to contribute it yourself.

Questions:

  • Should Tally (not Tallies) be iterable? What would you iterate over the cells?

@MicahGale
Copy link
Collaborator Author

unassigned @MicahGale

@MicahGale
Copy link
Collaborator Author

@bascandr your thought on converting types might not be great. But what if we gave you a way to make a "copy" that is another class?

@MicahGale
Copy link
Collaborator Author

In GitLab by @bascandr on Jan 5, 2024, 07:39

The ability to copy and re-class a tally as a single action could go a long way toward preventing many of the user error situations that I'm worried about here. I could also see this turning into the same function that changes the type of a tally simply by making a copy then deleting the original.

@MicahGale
Copy link
Collaborator Author

So implement something like:

tally.clone(new_type="F6")

and

tallies.convert_type(tally_number=1234, new_type="F7")

@MicahGale
Copy link
Collaborator Author

In GitLab by @bascandr on Jan 5, 2024, 10:44

I think that should work. It makes it very clear to the user how to accomplish those tasks and should avoid most attempts to do it in more problematic creative ways.

@MicahGale MicahGale self-assigned this Jan 12, 2024
@tjlaboss
Copy link
Collaborator

This no longer seems to be a good first issue!

@MicahGale
Copy link
Collaborator Author

... RIP. Yeah I think I need funding to be able to put together a good implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request An issue that improves the user interface. mccafe ☕ Issues that need to be resolved before the MCCAFE MontePy port
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants