Skip to content

Parsing of 'c' value as float instead of timedelta[ns]#402

Merged
rubencalje merged 5 commits intogwmod:devfrom
MattBrst:patch-1
Jan 16, 2025
Merged

Parsing of 'c' value as float instead of timedelta[ns]#402
rubencalje merged 5 commits intogwmod:devfrom
MattBrst:patch-1

Conversation

@MattBrst
Copy link
Contributor

By default the 'c' value was read as a timedelta[ns]. So a value of -9999 days was shown as -863913600000000000 nanoseconds and therefore not caught by the NaN replacement in the following lines.

By default the 'c' value was read as a timedelta[ns]. So a value of -9999 days was shown as -863913600000000000 nanoseconds and therefore not caught by the NaN replacement in the following lines.
@dbrakenhoff
Copy link
Collaborator

Nice catch! Maybe even better to confirm the dtype of the "c" variable before doing the conversion? This feels like one of those things that could change without us knowing.

So maybe we could add: if "c" in variables and ds["c"].dtype.type is np.timedelta64:

Copy link
Collaborator

@dbrakenhoff dbrakenhoff left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the PR! Will merge after tests complete.

@rubencalje
Copy link
Collaborator

Thanks for the PR!

It might be better to add decode_times=False to xr.open_dataset(). In this way, no data is read as datetimes. I do not believe there is any time-data in the regis-dataset. We may also remove decode_coords="all" from xr.open_dataset(), as I do not remember why we added this in the first place:

ds = xr.open_dataset(REGIS_URL, decode_times=False)

@rubencalje
Copy link
Collaborator

Thanks for the PR!

It might be better to add decode_times=False to xr.open_dataset(). In this way, no data is read as datetimes. I do not believe there is any time-data in the regis-dataset. We may also remove decode_coords="all" from xr.open_dataset(), as I do not remember why we added this in the first place:

ds = xr.open_dataset(REGIS_URL, decode_times=False)

Hmm, I changed this 4 months ago: 555e0f0. Not sure why anymore...

@rubencalje
Copy link
Collaborator

Hi @MattBrst,

Your PR has shown the decode_times=False line was there for a good reason. I added this again, and now your fix is not needed anymore.

It turns out we never used the variables argument, for example to get the values for the resistance c or the transmissivity kD. I made some changes, so you can also just retrieve the resistance c (which resulted in an error before):
regis = nlmod.read.regis.get_regis(extent, variables='c')

I kept decode_coords="all", as I am not entirely sure what it does.

If you are ok with the changes, we can merge this PR.

Copy link
Collaborator

@dbrakenhoff dbrakenhoff left a comment

Choose a reason for hiding this comment

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

Nice! Looks a lot more robust now :).

@rubencalje rubencalje merged commit a0300fe into gwmod:dev Jan 16, 2025
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