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

refactor(prt): deconflict naming, use enum for tracking level #2202

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

wpbonelli
Copy link
Contributor

@wpbonelli wpbonelli commented Feb 5, 2025

Particle[Store]Type%idomain has nothing to do with discretization IDOMAIN, name it itrdomain for "tracking domain" to clarify what we mean. Also use an enum for method tracking level.

@wpbonelli wpbonelli added the code refactor Nonfunctional changes label Feb 5, 2025
@wpbonelli wpbonelli added this to the 6.6.1 milestone Feb 5, 2025
Copy link
Contributor

@aprovost-usgs aprovost-usgs left a comment

Choose a reason for hiding this comment

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

great idea. just one comment - happy to chat

Copy link
Contributor

Choose a reason for hiding this comment

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

Enumerators are a great idea. My only question is where's the best place to define them. Ideally, the particle shouldn't have to know what kind of a domain it's being tracked though. (Hopefully that's true except for the new enumerators, but I'm not 100% sure to be honest.) Could they be defined in the relevant tracking methods? In addition to a desirable separation of concerns, this would also allow them to be customized for different kinds of tracking domains in the future, e.g., a stream discretized into reaches. Would be happy to chat if you think this is feasible and worth exploring

Copy link
Contributor Author

@wpbonelli wpbonelli Feb 6, 2025

Choose a reason for hiding this comment

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

Good point- agree that method is the better place to define domain level. Maybe instead of an enum, each level could then just be a constant defined in the corresponding base method type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure the particle module is a good place for the status and event enums though

Copy link
Contributor

Choose a reason for hiding this comment

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

totally agree - the status and event enums are best kept in the particle module

Copy link
Contributor

@aprovost-usgs aprovost-usgs Feb 6, 2025

Choose a reason for hiding this comment

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

regarding domain levels - so each method type would have its level baked in, perhaps as a %level attribute, instead of hard-coded numbers? seems like that would work

@wpbonelli
Copy link
Contributor Author

This one can wait until after the release, since it sounds like we want to rethink where to define tracking level constants. Maybe introduce a base method type for each level, like we just did for cells in #2203?

@wpbonelli wpbonelli marked this pull request as draft February 6, 2025 14:48
@wpbonelli wpbonelli added the onhold Waiting for something label Feb 6, 2025
@wpbonelli wpbonelli modified the milestones: 6.6.1, 6.7.0 Feb 6, 2025
@aprovost-usgs
Copy link
Contributor

This one can wait until after the release, since it sounds like we want to rethink where to define tracking level constants. Maybe introduce a base method type for each level, like we just did for cells in #2203?

maybe that's a good approach - agree it can wait, and let's think on it some more. i'm generally wary of adding too many classes, but sometimes it just makes sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code refactor Nonfunctional changes onhold Waiting for something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants