passaH2O / dorado

For shallow-water Lagrangian particle routing.
https://passah2o.github.io/dorado
MIT License
54 stars 9 forks source link

Add nourishment functions #20

Closed wrightky closed 3 years ago

wrightky commented 3 years ago

PR introduces three main feature additions:

I think the nourishment functions would make for a good new example, which I'd be happy to add to this PR before it's merged, if you agree that would be a worthwhile addition. If not, we can merge whenever things look good.

NourishmentArea

nourish_area_rcm

nourish_time

wrightky commented 3 years ago

Alright, I think this is ready for another look when you have a chance. In the latest commits, I have:

On that last point, that seems to be the only line with commits that conflict with the existing commits. Could use some help figuring out how to resolve that, still not very familiar with handling parallel commit histories.

elbeejay commented 3 years ago

On that last point, that seems to be the only line with commits that conflict with the existing commits. Could use some help figuring out how to resolve that, still not very familiar with handling parallel commit histories.

Sorry I missed addressing this point. GitHub has a reasonable conflict resolution built into the website, so I think you can do it in the PR by hitting the "Resolve conflicts" button and working through the GitHub GUI. Especially since the conflict is pretty minor.

Otherwise the other option (I'm familiar with) would be doing it via the command line by fetching the "current" version of the code and pulling it into your branch. Then the git CLI will prompt you to resolve the conflict there (which would only be the single line in that case as well).

Let me know if you want more info or want to go through it together and we can do that sometime.

wrightky commented 3 years ago
  • Does the nourishment area image need a colorbar? I don't mind either way but was just a thought.

I think probably not, since everything is normalized. Included one for the time function because the nominal values mean something, whereas area is all relative.

  • Add tests that use the smoothing in the nourishment calculations (sigma > 0)

Aside from being exhaustive, do you think there's any utility in doing this? When sigma > 0, it isn't easy to compute what the a priori answer should be without just using this function to compute it, so we'd essentially be forcing this function to give us the answer it already gave us.

  • test_nourishment_area() fails when I run the unit tests locally (seems to be producing some nan values in place of some expected 0s?)

This was a good catch, fixed the assert to handle floats or nan

  • Do we know if get_time_state() is working as intended? Doesn't have to be resolved by this PR if there is an issue but we should open up an issue if there is a known bug

All my local testing of the new version works, there's very little that can go wrong now that we've collapsed the main loops/ifs into one simple numpy search. But I still don't know if this is what was giving Nelson problems

elbeejay commented 3 years ago
  • Does the nourishment area image need a colorbar? I don't mind either way but was just a thought.

I think probably not, since everything is normalized. Included one for the time function because the nominal values mean something, whereas area is all relative.

:+1:

  • Add tests that use the smoothing in the nourishment calculations (sigma > 0)

Aside from being exhaustive, do you think there's any utility in doing this? When sigma > 0, it isn't easy to compute what the a priori answer should be without just using this function to compute it, so we'd essentially be forcing this function to give us the answer it already gave us.

I think it is worth doing (beyond being exhaustive) for the following 2 reasons:

  1. Avoids any possibility of an errant typo or line-change accidentally disrupting the function or performance of that section of the code that slips by our visual check of a future PR
  2. Will throw an error if one of the functions from an upstream library (scipy.ndimage.gaussian_filter in this case) is changed in a way that breaks this functionality or alters the output; whether that is changing the number or order of expected inputs/outputs or altering the function internals and the subsequent output. Then we'd have a chance to do any/all of the following: 1) pin the scipy version to one that works; 2) update the function to reflect the new scipy syntax; 3) if the result of the function call changes we could add a warning in our code to let users know their result might change depending on the version of scipy they have (admittedly for this function this is unlikely).
  • test_nourishment_area() fails when I run the unit tests locally (seems to be producing some nan values in place of some expected 0s?)

This was a good catch, fixed the assert to handle floats or nan

:+1:

  • Do we know if get_time_state() is working as intended? Doesn't have to be resolved by this PR if there is an issue but we should open up an issue if there is a known bug

All my local testing of the new version works, there's very little that can go wrong now that we've collapsed the main loops/ifs into one simple numpy search. But I still don't know if this is what was giving Nelson problems

Sounds good.

wrightky commented 3 years ago

Think this might be ready!