Warn users that tally heating score with photon bin but without electron and positron bins.

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040

[GuySten]

Description

Because OpenMC does not transport charged particles and other than heating there are no scores that use charged particles bins,
users might forget to tally positron and electron bins when calculating heating score and using particle filter.
This PR add a warning for that case.

Examples where user forgot to tally heating charged particles bins:
https://openmc.discourse.group/t/photon-heating-discrepancies-with-mcnp-and-open-mc/5705
https://openmc.discourse.group/t/benchmark-against-mcnp-and-tripoli4-unsatisfactory/3154

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[shimwell]

Great idea. Keen to see this merged. Wondering if the warning can be more specific about what the user needs to do to fix. perhaps it could go as far as giving the python code snippet of what is needed.

[GuySten]

@shimwell, I welcome suggestions for a better warning message if you have any.

[shimwell]

how about something along these lines. I find it super useful when i can copy paste the solution from the error message

"Tally {} contains heating score with photon bin but "
"without electron bin. Try adding to the particle filter "
"openmc.ParticleFilter(['photon', 'electron', 'positron']) "

[GuySten]

@shimwell, I've changed the error message.
Could you review this PR?

[shimwell]

I like this idea.

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I think get_filter returns the first particle filter in this case and not all the particle filters

[GuySten]

I'm not sure the current implementation will work properly if there are multiple ParticleFilters in the tally.filters

I don't think it is legal to have multiple filters of the same type.

[shimwell]

thanks for the improving the user experience and answering my only concern. Happy to see this merged

[makeclean]

Silly question, is there a precedent for this elsewhere in the C++ layer? I'm somewhat anxious having an error message this is not explicitly related to the route that someone used to produce the error, i.e. one could be executing having written the xml (rather than via the python layer) and this wouldn't really be relevant (nor paste-able - not that we should be pasting stuff into xml files).

Should this error not be raised by the python layer when you instantiate the tally object? You can more safely iterate over the various member objects of the python classes making sure that as particles are added there is no need to duplicate effort in the C++ class?

I guess I worry that there should be a separation of concerns here, we could quickly spiral into error messages being influenced by the mutable python interface, which can be out of step relative to the C++ layer.

[shimwell]

ah yes fair point. A check that returns python code as the fix could be done by the openmc.ParticleFilter() at the python level. I guess this is my fault for asking for the warning message to be change to include the python code in the first place.

[GuySten]

The problem with checking that on the python side is that the python state is dynamic.
You can add attributes to the tally object in any order so it is not clear to me where is the right place to add the check.

In the cpp side as long as we do not use the C-API it is natural to place that warning because we are guaranteed to have all needed data.

For example you can set particle filter and then heating score or the other way around.
I think you can also have a workflow where you set an empty particle filter and add bins to it afterwards.
So detection of this warning on the python side will be really complicated.

That is why I prefer a check on the cpp side.

[shimwell]

Well I guess you are both right so the common ground is to remove the part of the error message I thought was useful so it is not longer python specific 😭

[paulromano]

I generally prefer to have checks on the Python side because coding wise they just are simpler, and less stuff to compile. That being said, I'm actually not a huge fan of putting warnings in for every little thing because there are a million ways for users to shoot themselves in the foot (by design) and it's not really our job to tell them every little thing they are doing wrong. In principle, there is nothing "wrong" with having a ParticleFilter with only "photon" and tallying heating -- that might be what the user wants. Personally I think a better option here is to have clearer documentation (e.g., on ParticleFilter and in the user's guide).

[paulromano]

I'll also add that I'm considering changing this behavior so that users do get all the heating lumped in the photon bin, provided that electrons and positrons are not being transported (which may be possible in the future).

[GuySten]

I've opened this PR because this issue is the biggest silent footgun I am familiar with in openmc.
I am also ok with adding a Common Pitfalls section in the documentation.
@paulromano, If you have a list of common footguns in openmc I will be happy to write that section.

In any case, if you are considering to change the behavior to score electron and positron heating in the photon bin
I think this PR should be on hold anyway.

[GuySten]

Superseded by #4040