Skip to content

Add optional parameters to walk data and a new function related to seeding#60

Merged
elbeejay merged 18 commits into
passaH2O:masterfrom
eghenson:master
May 26, 2026
Merged

Add optional parameters to walk data and a new function related to seeding#60
elbeejay merged 18 commits into
passaH2O:masterfrom
eghenson:master

Conversation

@eghenson

Copy link
Copy Markdown
Contributor

This PR includes 3 adjustments to dorado.

  1. Updates the particle tracking output system to allow variables already stored in the modelParams class (e.g., depth, stage, qx, v, etc.) to be recorded in walk data.

These variables can now be selected as particle outputs by specifying them in params.particle_variables when initializing the model.

  1. Introduces a new particle variable:

roi_flag — a binary flag (0/1) indicating whether a particle step occurs within a specified roi grid, set by params.roi_grid

  1. Adds a new function:

flux_proportional_seeding in particle_track.py

This function supports unsteady particle seeding by allocating the number of particles seeded at each timestep proportional to the discharge magnitude at that timestep. It requires a discharge array preprocessed to match the seeding timesteps at the seeding location.

These additions allow users to record additional variables along particle trajectories for various purposes. If no params.particle_variables are set, the default should be the normal walk data comprised of xinds, yinds, travel_times.

I have used these functionalities to output depth and roi_flag at each particle step to estimate potential nutrient processing within selected regions of a domain.

Looking forward to feedback!

@codecov

codecov Bot commented Mar 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.74359% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.03%. Comparing base (4c9f638) to head (fa64f59).

Files with missing lines Patch % Lines
dorado/particle_track.py 88.05% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
- Coverage   95.64%   95.03%   -0.61%     
==========================================
  Files           4        4              
  Lines         712      786      +74     
==========================================
+ Hits          681      747      +66     
- Misses         31       39       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@elbeejay
elbeejay requested review from elbeejay and wrightky March 11, 2026 01:24
@eghenson

Copy link
Copy Markdown
Contributor Author

Hello, all. I believe I have addressed the original issues in the get_state function to better handle the optional parameter outputs. I also added some variable testing in test_particle_track.py and reran the test suite locally.

I am not sure what the two codecov/ failing checks are referring to.

Aside from that, @elbeejay I just chatted with @wrightky about the final update remaining, which would be adjusting parallel_routing.py to handle optional outputs. I have not run the parallel routing myself, but I understand it should be updated for consistency with running in serial. I can take a stab at updating that as well, if we think that would be best.

One other small thing I noticed in parallel_routing.py: I found that the walk data keys xinds and yinds (as written in the rest of the code) is written as x_inds and y_inds. Though travel_times is written consistently across both. @elbeejay was this intentional, or should I adjust the key names if I am to try to update this file?

Thanks!

@elbeejay

Copy link
Copy Markdown
Member

Hello, all. I believe I have addressed the original issues in the get_state function to better handle the optional parameter outputs. I also added some variable testing in test_particle_track.py and reran the test suite locally.

I am not sure what the two codecov/ failing checks are referring to.

They are tracking the test "coverage" which is defined by codecov as the fraction of lines of code that the unit tests execute. Those two are failing because because the altered code in the diff is 73.25% covered by the tests while the pre-existing version of the code is 95.65% covered by the tests. The second one just indicates the merging the changes would drop coverage 2.45%. All that being said, fraction of lines covered by the tests isn't necessarily the "best" measure of software testing; it is entirely possible to "cover" 100% of the codebase with meaningless tests. Just wanted to give you the context on those checks.

Aside from that, @elbeejay I just chatted with @wrightky about the final update remaining, which would be adjusting parallel_routing.py to handle optional outputs. I have not run the parallel routing myself, but I understand it should be updated for consistency with running in serial. I can take a stab at updating that as well, if we think that would be best.

If you have the time that sounds great!

One other small thing I noticed in parallel_routing.py: I found that the walk data keys xinds and yinds (as written in the rest of the code) is written as x_inds and y_inds. Though travel_times is written consistently across both. @elbeejay was this intentional, or should I adjust the key names if I am to try to update this file?

Yep if you're in there and want to make that update so the keys are consistent across the serial and parallel implementations that would be fantastic. 👍

@elbeejay elbeejay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like a positive addition to the package @eghenson. Definitely some nice new functionality. I have a couple of questions in addition to my review comments:

  • What do you think about adding tests for the new flux_proportional_seeding function to the test_particle_track.py script?

  • Are you planning to add or expand on any example scripts to showcase how this new functionality can be used?

Also as a note, before this is merged (once ready) we need to remember to increment the version number of the package in the __init__.py and pyproject.toml files (#51)

Comment thread dorado/particle_track.py Outdated
Comment thread dorado/particle_track.py Outdated
Comment thread dorado/lagrangian_walker.py Outdated
Comment thread dorado/particle_track.py
Comment thread dorado/particle_track.py Outdated
@eghenson

Copy link
Copy Markdown
Contributor Author

Looks like a positive addition to the package @eghenson. Definitely some nice new functionality. I have a couple of questions in addition to my review comments:

  • What do you think about adding tests for the new flux_proportional_seeding function to the test_particle_track.py script?

I added some tests here - let me know if you think there should be a more advanced one, @elbeejay !

  • Are you planning to add or expand on any example scripts to showcase how this new functionality can be used?

I also added a short example script called particle_flagging_with_additional_outputs.ipynb that used the unstructured ANUGA data. Let me know your thoughts on this.

Also as a note, before this is merged (once ready) we need to remember to increment the version number of the package in the __init__.py and pyproject.toml files (#51)

@eghenson
eghenson requested a review from elbeejay March 25, 2026 11:17

@elbeejay elbeejay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@eghenson - I looked at the revisions and I think things look good. Pending a once-over by @wrightky I think we're good to merge.

@elbeejay

Copy link
Copy Markdown
Member

Not going to hold this up any longer...

@elbeejay
elbeejay merged commit 16fdb26 into passaH2O:master May 26, 2026
8 of 10 checks passed
@wrightky

Copy link
Copy Markdown
Collaborator

Thanks @elbeejay! Apologies for not following up on this, I've looked it over a few times now and kept getting distracted before finishing the merge, but everything looks good to me.

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.

3 participants