Allows DatasetRestoring to act on fields by different name

[jagoosw]

This PR moves variable_name to be a kwarg in DatasetRestoring to allow users to override the default naming.

Also closes #61

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/DataWrangling/ECCO/ECCO_darwin.jl	0.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[simone-silvestri]

I don't think we can pass a varname that is different than the one in metadata, while this keyword argument would allow it. I would probably drop the variable_name kwarg, what you want to achieve can be done just by passing field_name am I right?

[jagoosw]

I don't think we can pass a varname that is different than the one in metadata, while this keyword argument would allow it. I would probably drop the variable_name kwarg, what you want to achieve can be done just by passing field_name am I right?

Oh yes thats true

[jagoosw]

Could I add a scale factor to the dataset? Do you think it would be best to do it in the constructor, or to pass a scale factor to the forcing function?

[simone-silvestri]

there is already the rate and mask kwargs. Would the scale be different?

[jagoosw]

there is already the rate and mask kwargs. Would the scale be different?

I mean if the dataset is different (e.g. in different units) to the field its restoring, so it would go in as s in $r (s\psi_r - \psi)$

[simone-silvestri]

We have a conversion_units and a convert_units to convert data units to the model required units. We could do it through there?

[jagoosw]

We have a conversion_units and a convert_units to convert data units to the model required units. We could do it through there?

Doesn't that do a fixed conversion depending on the dataset? Or is the a user way to change the conversion?

[jagoosw]

Circling back to this issue: I think it would be good if users could pass a "conversion unit" to Metadata incase their model etc. is using different units. Its also a bit confusing what the "native" unit being converted to is so it might be more clear to e.g. write MolPerLiterToMilliMolPerLiter etc. even though its longer?

[simone-silvestri]

The units system is a bit rough around the edges. @bgroenks96 proposed refactoring it using https://github.com/JuliaPhysics/Unitful.jl.

I think it would be good if users could pass a "conversion unit" to Metadata incase their model etc. is using different units.

For physics quantities we probably do not want to change the units because the interface fluxes enforce a certain convention (temperature in kelvin, salinity in psu, velocities in m/s and so on). I can imagine that for tracers each model might want to have its own convention. We will need to enforce some type of consistency with what the flux solver sees though.

[jagoosw]

The units system is a bit rough around the edges. @bgroenks96 proposed refactoring it using https://github.com/JuliaPhysics/Unitful.jl.

I saw this which seems like a good idea but a lot of work.

For physics quantities we probably do not want to change the units because the interface fluxes enforce a certain convention (temperature in kelvin, salinity in psu, velocities in m/s and so on). I can imagine that for tracers each model might want to have its own convention. We will need to enforce some type of consistency with what the flux solver sees though.

Yeah that makes sense r.e. physical quantities. People use different units for different BGC models (e.g. mol C vs mol N) so I think theres going to frequently be cases where you might want to change units for restoring, but thats going to make a challenge to enforce correct units when necessary for fluxes. Could I merge this PR and open a new one to think about that?