search

COSIMA / cosima-cookbook

Framework for indexing and querying ocean-sea ice model output.
https://cosima-recipes.readthedocs.io/en/latest/
Apache License 2.0
58 stars 25 forks source link

Require specified attributes to be unique to ensure disambiguation and consistent data sets returned #255

Closed aidanheerdegen closed 3 years ago

aidanheerdegen commented 3 years ago

Closes #252

Added argument to getvar and _ncfiles_for_variable to specify attributes that are required to be unique to ensure data returned is consistent and correct.

aidanheerdegen commented 3 years ago

This is still a draft as I have to update some tests and add new tests.

aidanheerdegen commented 3 years ago

@angus-g would be grateful for some feedback about how I am doing this. Wouldn't like to change all the tests for no reason if there is a better way.

aekiss commented 3 years ago

@aidanheerdegen one wrinkle is JRA55 data, which has cell_methods = "area: time: mean" https://github.com/COSIMA/cosima-cookbook/pull/214#issuecomment-778115202

aidanheerdegen commented 3 years ago

Another question, should it be a warning or exception if duplicate values for a disambiguation attribute are detected?

We definitely don't want users to accidentally load incorrect data, so an exception would make sure this doesn't happen. It is unlikely to happen unless they change the existing default parameters with the current configuration of variables, but new variables/datasets could be added which do not play by the currently constituted "rules".

Thoughts @aekiss?

aekiss commented 3 years ago

Another question, should it be a warning or exception if duplicate values for a disambiguation attribute are detected?

We definitely don't want users to accidentally load incorrect data, so an exception would make sure this doesn't happen. It is unlikely to happen unless they change the existing default parameters with the current configuration of variables, but new variables/datasets could be added which do not play by the currently constituted "rules".

Thoughts @aekiss?

I'm in two minds about this. But maybe an exception is safest? It should be a rare occurrence, and a user could override it with attrs_unique={} if they really need to (maybe this tip could be added to the warning/exception message?)

aekiss commented 3 years ago

Looks like this PR will resolve https://github.com/COSIMA/cosima-cookbook/issues/137 as well as #252

aidanheerdegen commented 3 years ago

Took a look at #137 and it seems to be more about differing grids with regional output, and in any case some of those regional files seem broken (time axis is 0 length)

aidanheerdegen commented 3 years ago

Damn. Forgot to merge fixes indentified by @aekiss. Will put in another small PR.

navidcy commented 3 years ago

Great! Let's close the other issues (in other repos also).

aidanheerdegen commented 3 years ago

Which ones @navidcy ? #234? That is your issue, so close it if you think it has been sorted. Seems so, but I'll leave it to you.

137 is a slightly different issue, but if @aekiss thinks it is no longer relevant he is free to close it.

navidcy commented 3 years ago

COSIMA/access-om2#234 ?

aidanheerdegen commented 3 years ago

234? That is your issue, so close it if you think it has been sorted. Seems so, but I'll leave it to you.