openmc icon indicating copy to clipboard operation
openmc copied to clipboard

Add ambient dose coefficients (H*(10)) from ICRP74

Open MatteoZammataro opened this issue 1 year ago • 7 comments

Description

This pull request is in response to issue #3216, and is a supplement to PR #3020. The goal is to allow the user to use H*(10) dose coefficients in addition to the effective dose coefficients which were previously available. I've created a folder called ambient_dose_coefficients on openmc.data, in which I've defined the function ambient_dose_coefficients(). The previous function dose_coefficients() has been renamed effective_dose_coefficients(). I have kept the same folder architecture used in effective_dose in case there is a possibility to add more coefficients from different data sources.

Checklist

  • [x] I have performed a self-review of my own code
  • [x] I have followed the style guidelines for Python source files
  • [x] I have made corresponding changes to the documentation
  • [x] I have added tests that prove my fix is effective or that my feature works

MatteoZammataro avatar Jan 09 '25 16:01 MatteoZammataro

The ambient dose equivalent is absolutely necessary for safety analysis related works and comparison with dose measurement. But do not the changes of the dose_coefficients() function and related module names break the backward compatibility?

vitmog avatar Nov 26 '25 21:11 vitmog

The ambient dose equivalent is absolutely necessary for safety analysis related works and comparison with dose measurement. But do not the changes of the dose_coefficients() function and related module names break the backward compatibility?

Thanks for pointing that out. You're actually right: renaming dose_coefficients() does technically break backward compatibility. I’m happy to adjust the implementation if you have a preferred approach. Let me know what direction you recommend and I’ll update the PR accordingly.

MatteoZammataro avatar Nov 27 '25 08:11 MatteoZammataro

Thanks for the PR, I think this would be a useful and popular addition

If you are keen to rename a function then one way to help users currently using that function would be to keep the old function and add a deprecated warning when users call it and then within the old function call the new function. There are some examples in the code base if that helps.

Personally I am in favour of really small PRs that are very focused on a single issue. So if it is possible I wouldn't change the dose_coefficients in this function name in this PR and I would keep the PR focused on adding the H*(10) coefficients. I think this will make it easier to review and merge. Then if you are keen follow up the PR with one that renames the function and adds a deprecated warning and the pros and cons of that name change can be considered separately to the H*(10) feature.

shimwell avatar Nov 27 '25 09:11 shimwell

I’m happy to adjust the implementation if you have a preferred approach.

~~@MatteoZammataro I do not see any dilemma because the effective and ambient equivalent doses do not overlap by the publication parameter data_source: effective ones are always from ICRP (biological) publications, but ambient ones are from ICRU (instrumental) ones. The both are different commissions with different funding, I would likely trust in they will have created an additional new commission or, better, several ones than to merge each with other.~~

~~Following the existent naming convention, those can be designated as icrp74 , icru57, etc. So I just meant to keep the name of function and leave it as is.~~

~~Also, for the ambient equivalent it is possible to provide an identifier of what a kind of the ambient dose equivalent is computed exactly analogously to the effective dose cases AP, PA, etc. So, again, just following the existent convention does not seems being a bad solution.~~

UPD. Mistake, my bad.

vitmog avatar Nov 27 '25 12:11 vitmog

@MatteoZammataro I am sorry for the produced mistake: actually effective and ambient dose equivalents overlaps by the data_source argument in the case of ICRP-74.

vitmog avatar Nov 27 '25 14:11 vitmog

Thanks for the PR, I think this would be a useful and popular addition

If you are keen to rename a function then one way to help users currently using that function would be to keep the old function and add a deprecated warning when users call it and then within the old function call the new function. There are some examples in the code base if that helps.

Personally I am in favour of really small PRs that are very focused on a single issue. So if it is possible I wouldn't change the dose_coefficients in this function name in this PR and I would keep the PR focused on adding the H*(10) coefficients. I think this will make it easier to review and merge. Then if you are keen follow up the PR with one that renames the function and adds a deprecated warning and the pros and cons of that name change can be considered separately to the H*(10) feature.

I’m happy to adjust the implementation if you have a preferred approach.

@MatteoZammataro I do not see any dilemma because the effective and ambient equivalent doses do not overlap by the publication parameter data_source: effective ones are always from ICRP (biological) publications, but ambient ones are from ICRU (instrumental) ones. The both are different commissions with different funding, I would likely trust in they will have created an additional new commission or, better, several ones than to merge each with other.

Following the existent naming convention, those can be designated as icrp74 , icru57, etc. So I just meant to keep the name of function and leave it as is.

Also, for the ambient equivalent it is possible to provide an identifier of what a kind of the ambient dose equivalent is computed exactly analogously to the effective dose cases AP, PA, etc. So, again, just following the existent convention does not seems being a bad solution.

Thanks so much for the feedbacks! Much appreciated. I’ve kept the function name as data.dose_coefficients, and I added the dose_type argument to switch between effective and ambient dose coefficients.

For the longer term, it might be clearer to rename the effective_dose folder to something more general like dose, since it now contains both effective and ambient dose data.

Please let me know if you have any other suggestions!

MatteoZammataro avatar Nov 27 '25 14:11 MatteoZammataro

@MatteoZammataro Thank you for the pushing this topic! Using the dose_type argument for the switch seems quite good to me. The idea about the folder effective_dose renaming sounds absolutely natural and logical. However, I assume that the issue of the break backward compatibility should be by principal developers.

vitmog avatar Nov 27 '25 15:11 vitmog