EcohydrologyTeam / ClearWater-riverine

A 2D water quality transporter model to calculate conservative advection and diffusion of constituents from an unstructured grid of flows
MIT License
6 stars 0 forks source link

Issue with link between riverine and modules #80

Open sjordan29 opened 2 weeks ago

sjordan29 commented 2 weeks ago

@jrutyna worked on coupling the new constituents-dev branch with clearwater-modules and the impacts of TSM were visible in the reaction_model xarray but not the transport_model xarray.

Looking into this, I think the issue is here, particularly the part and constituent in update_concentration.keys(). I think constituent here is an instance of the Constituent class, which would never been in the list of user-provided update_concentrations.keys(). I think what happened is we never satisfied the second part of this conditional so Riverine just kept chugging away with out any user input. We'll want to do two things here:

  1. I believe we'll want to put constituent_name here in place of constituent
  2. Issue a warning to users if they provided a key in their update_concentrations dictionary that isn't one of the constituents in the model, so that future users won't run into this silent error #79

the way I'd go about confirming this:

  1. Use the 30-second model for troubleshooting since it instantiates fastest
  2. Make a simple .py script that instantiates clearwater riverine and updates the model with the update_concentration parameter. I'd just fill this with some arbitrary value, something like (haven't tested, so may need to fix some bugs):
    
    arbitrary_values = xr.full_like(transport_model.mesh.temperature, 5000) # make an array the shape of the temp array with 5,000 everywhere
    update_concentration = {'temperature': arbitrary_values}
    transport_model.update(update_concentration)

3. Run with the debugger with breakpoints throughout the `update` function of Riverine to confirm that the temperature is getting updated. 
4. Once you feel good it's fixed, try running the full simulation coupled with Modules with the 30-second model and confirm you see the signature in the transport_model mesh.
5. Then we could try with 1-second. 

Note: there's a `finalize` function in CW-R [here](https://github.com/EcohydrologyTeam/ClearWater-riverine/blob/constituents-dev/src/clearwater_riverine/transport.py#L327) where you can indicate that you want to save the output (boolean input) and you can then specify a filepath to save your model output. 
jrutyna commented 2 weeks ago

@sjordan29, I added the warning print statement with this commit: 5c69fa8

jrutyna commented 2 weeks ago

@sjordan29, I fixed the bug in the warning print statement when the model is coupled with TSM. I also tested the finalize function and saved output with the zarr format, but the netCFD format is giving an error. I have the 1-second model up and running (will post an update when the model is finished).

jrutyna commented 2 weeks ago

@sjordan29, please see this commit for the above comment: 5efd5f8

sjordan29 commented 2 weeks ago

Gotcha, looks like for netcdf output, the volume_calculation_required attribute of the xarray is not allowed (I believe that's a boolean, and it must be invalid datatype for netcdfs but not zarr). There are a few alternative ways we could address this

  1. Change that from a boolean to a string [either in the finalize function OR in the io/output module for netcdf files only
  2. In the save_clearwater_xarray method in the mesh module, I think we'd just need to add that to the list of attributes to delete (or, again, we could do this in the io/output module for netcdf files only).

I can implement this next week if you don't get to it.