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

ENH: Support passing depletion reader settings at construction #516

Merged

Conversation

drewejohnson
Copy link
Collaborator

@drewejohnson drewejohnson commented Jun 3, 2024

Not really a deprecation yet but this is my first step into #339. The depletion reader can be fully configured at construction like

r = DepletionReader(
    "case_dep.m",
    materials=["f.*"],
    materialVariables=["ADENS", "INH_TOX"],
    metadataKeys=["NAMES", "DAYS"]],
    processTotal=False,
)
r.read()

All of the arguments are optional, defaulting to None. If not given, the values in the rc global settings object is used.

Starting small because I want to get buy in from @DanKotlyar and other users before applying this approach to all the readers.

Current plan roll out (to be re-posted into #339) is

  1. next release : each reader can be fully configured at construction. rc unchanged
  2. +1 major release : setting any value in the rc object raises a deprecation warning. transition tests away from using rc in favor of direct construction
  3. +3 major releases : remove the rc object from the code base

This should give enough time for users to update their applications to move away from the rc object

Make sure you have read over the developer guide to ease
the review process. These include:

TODO

  • Add something to the changelog (0.11.0?)
  • Add a versionadded tag in the docstrings for these parameters

@drewejohnson drewejohnson self-assigned this Jun 3, 2024
DanKotlyar
DanKotlyar previously approved these changes Jun 5, 2024
Copy link
Contributor

@DanKotlyar DanKotlyar left a comment

Choose a reason for hiding this comment

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

I like this approach a lot.

@drewejohnson
Copy link
Collaborator Author

@DanKotlyar thanks! Let's let this sit for a few more days to try to get some more feedback. I think this is a good step, but want to give people the chance to chime in on the potential removal of the global rc object

@drewejohnson
Copy link
Collaborator Author

@DanKotlyar ready for your re-review and merge if approved

@DanKotlyar DanKotlyar merged commit e7d97bf into CORE-GATECH-GROUP:main Jun 12, 2024
4 checks passed
@drewejohnson drewejohnson added this to the 0.11.0 milestone Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants