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

Remove gempyor.seeding Dependence On gempyor.model_info.ModelInfo #422

Merged
merged 7 commits into from
Jan 15, 2025

Conversation

TimothyWillard
Copy link
Contributor

Describe your changes.

This pull request untangles gempyor.seeding.Seeding from gempyor.model_info.ModelInfo by requiring explicit values be passed to gempyor.seeding.Seeding. This eases the unit testability of the gempyor.seeding module. Also adds the ModelInfo.get_seeding_data method which replaces prior calls to the seeding instance from the Modelnfo instance with a modinf argument.

Does this pull request make any user interface changes? If so please describe.

The user interface changes are:

  • Added gempyor.model_info.ModelInfo.get_seeding_data method,
  • Changed interface for gempyor.seeding.Seeding.get_from_config/file methods, and
  • Deprecated gempyor.seeding.Seeding.get_from_file in favor of gempyor.seeding.Seeding.get_from_config.

Those are reflected in updates to the documentation in the docstrings of these methods.

What does your pull request address? Tag relevant issues.

This pull request addresses part of GH-397.

* Added explicit exports,
* Reordered imports,
* Added general structure comments, and
* Refactored all lines to be less than 92 characters.
Remvoed the `modinf` arg from the `_DataFrame2NumbaDict` internal
utility and replaced it with the individual attributes from the `modinf`
object that are needed.
Untangled the `Seeding` class from the `ModelInfo` class by adding a
method to `ModelInfo` that interfaces with the `Seeding` class.
Updated remaining references in inactive code, notebooks and the
`gempyor.dev` module.
@TimothyWillard TimothyWillard added 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 10, 2024
@TimothyWillard TimothyWillard added this to the Software Quality milestone Dec 10, 2024
@TimothyWillard TimothyWillard added the next release Marks a PR as a target to include in the next release. label Dec 16, 2024
@TimothyWillard TimothyWillard marked this pull request as ready for review December 16, 2024 22:16
Copy link
Contributor

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

Looks mostly good, some minor adjustments suggested.

flepimop/gempyor_pkg/src/gempyor/inference.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/seir.py Outdated Show resolved Hide resolved
@pearsonca
Copy link
Contributor

Documenting my opinion re "why" argument= style - until we have a high degree of coverage of type annotations (and even after that, since the type checker isn't all-the-time handy), the dynamic polymorphism / argument introspection / etc for python (and similarly, R) is a minefield for mis-specification / mis-understanding / mis-use of arguments.

For quick-and-dirty-one-off scripts, the polymorphic behavior is great(*until it isn't, but it is generally faster to write). For libraries, however, we want to avoid setting ourselves traps. Even one-argument methods might in the future be revised to have more or in a different order or ...etc. When comparing across diffs here, keeping the argument names makes it easier to grok what's happening.

To the extent that code feels noisy, verbose, ... - I agree, but the fix to that is better abstractions, not using syntax sugar likely to lead to a hard-to-find bug in future work.

Copy link
Contributor

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

Looks good aside from one missed arg= revision

@TimothyWillard TimothyWillard force-pushed the feature/397/remove-seeding-dependence-on-modinf branch 2 times, most recently from 1f24393 to fa7c048 Compare January 8, 2025 18:15
* Per @pearsonca, @jcblemai suggestions call positional arguements with
  names.
* Restyle call to `get_seeding_data` in `gempyor.seir.onerun_SEIR`.
@TimothyWillard TimothyWillard force-pushed the feature/397/remove-seeding-dependence-on-modinf branch from fa7c048 to 085beca Compare January 8, 2025 19:04
@TimothyWillard TimothyWillard merged commit 5e6e622 into dev Jan 15, 2025
5 checks passed
@TimothyWillard TimothyWillard deleted the feature/397/remove-seeding-dependence-on-modinf branch January 15, 2025 14:17
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. enhancement Request for improvement or addition of new feature(s). gempyor Concerns the Python core. medium priority Medium priority. next release Marks a PR as a target to include in the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: cyclic dependencies between model_info, seeding, and initial_conditions complicate type hinting
4 participants