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

disallow NaN in ncread if allow_missing=false #186

Merged
merged 2 commits into from
Jan 7, 2022
Merged

disallow NaN in ncread if allow_missing=false #186

merged 2 commits into from
Jan 7, 2022

Conversation

visr
Copy link
Member

@visr visr commented Dec 24, 2021

If the fill value of the netCDF is NaN, NCDatasets will make it a missing. But if that is not the case and we don't want missings, I cannot think of any case where we would want NaN.

This will work for instance to catch NaN values if they are present in the river width with the current call to ncread:

ncread(nc, param(config, "input.lateral.river.width"); type = Float, fill = 0)

@visr visr requested a review from verseve December 28, 2021 10:17
Copy link
Member

@verseve verseve left a comment

Choose a reason for hiding this comment

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

I guess this PR is related to the NaN values that were caused by the resolved issue #187?

Although, on the one hand I think it is good to check for NaN values, on the other hand I think it is very rare that this occurs (or does not occur at all). For example, the maps in staticmaps.nc (from HydroMT) all have a _FillValue, so NaN is not explicitly defined in staticmaps.nc. For the river part a check for zero values in:

riverwidth = riverwidth_2d[inds]

riverlength = riverlength_2d[inds]

would make more sense I think. These kind of inconsistencies in the input data will result in NaN values when running the model (e.g. division by zero).

So, my suggestion is to leave this part out of the code, and to include for example checks for zero values in river width and length.

@visr
Copy link
Member Author

visr commented Jan 7, 2022

It was triggered by but is not really related to #187. You're right that for HydroMT staticmaps this is not needed, but I see it as an extra defense against missing/NaN data entering and the calculation and causing issues further in the code. Also netCDF files with a (non-NaN) fill value can have NaN values inside, which would currently just slip past the initialization code.

Your suggestion on the river lengths is added now in c42f78f.

Copy link
Member

@verseve verseve left a comment

Choose a reason for hiding this comment

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

Thanks for the addition in c42f78f!

Yes, let's keep it then as an extra defense mechanism against missing/NaN data entering during initialization. It can indeed happen when input files are edited or generated without HydroMT.

What about updating the changelog?

@visr
Copy link
Member Author

visr commented Jan 7, 2022

I'd say these kind of changes are not really important/relevant enough for users. Before and after this would throw an error. Perhaps at most we could add a general "Improved error messages" line.

@verseve
Copy link
Member

verseve commented Jan 7, 2022

Sounds good, I agree, an update of the changelog is not necessary.

@visr visr merged commit 115a6fc into master Jan 7, 2022
@visr visr deleted the ncnan branch January 7, 2022 14:11
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.

2 participants