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 request]: Refactor gempyor.NPI Into gempyor.modifiers #409

Open
TimothyWillard opened this issue Dec 3, 2024 · 1 comment
Open
Labels
docstring Relating to in-code documentation. documentation Relating to ReadMEs / gitbook / vignettes / etc. enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. medium priority Medium priority.

Comments

@TimothyWillard
Copy link
Contributor

Label

documentation, docstring, enhancement, gempyor

Priority Label

medium priority

Is your feature request related to a problem? Please describe.

The modifiers code is currently located under the gempyor.NPI submodule for legacy reasons and contains duplicated code that is under tested and under documented. This makes working with the functionality contained here challenging, it's difficult to know if changes break existing behavior or has already been implemented elsewhere as well as makes it difficult to share code without creating circular dependency issues.

Is your feature request related to a new application, scenario round, pathogen? Please describe.

No response

Describe the solution you'd like

Ultimately move gempyor.NPI and all of its contents into a consolidated, tested, and documented gempyor.modifiers module. The current setup with an ABC that modifiers extend is good, the refactored Modifier ABC needs to have:

  • An __init__ method that could build the modifiers from the properties of the class so subclasses don't have to have their own __init__ methods. (Including determination of relevant subpops, see Merge emcee_batch Into dev #404 (comment))
  • Have get_reduction abstract method that subclasses have to implement.
  • get_reduction_df that is a thin wrapper around get_reduction (unclear if get_reduction can just cover this or not).

A first step before making these larger end goal changes though would be to document and unit test the existing functionality. This will make migrating/refactoring much easier and safer to do.

@TimothyWillard TimothyWillard added documentation Relating to ReadMEs / gitbook / vignettes / etc. enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. medium priority Medium priority. docstring Relating to in-code documentation. labels Dec 3, 2024
@jcblemai
Copy link
Collaborator

jcblemai commented Dec 3, 2024

Just as a suggestion: one might also want modifiers to be a single class instead of the current architecture that loads several objects. This would allow easier plotting and debugging and all.

Some code to apply modifiers is located in parameters.py and in utils.py (I think, or in some file in the NPI/ folder)

@TimothyWillard TimothyWillard added this to the Software Quality milestone Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docstring Relating to in-code documentation. documentation Relating to ReadMEs / gitbook / vignettes / etc. enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. medium priority Medium priority.
Projects
None yet
Development

No branches or pull requests

2 participants