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

fixed probable typo for 'coordinate' entry in dimensions dictionary #236

Merged
merged 1 commit into from
Nov 16, 2020

Conversation

ifenty
Copy link
Contributor

@ifenty ifenty commented Nov 11, 2020

convention here seems to be

XC=dict(dims=["j", "i"], attrs=dict(
    standard_name="longitude", long_name="longitude",
    units="degrees_east", coordinate="YC XC")),

some instances fixed where "coordinate" was spelled "coordinates". May have no effect.

convention here seems to be

    XC=dict(dims=["j", "i"], attrs=dict(
        standard_name="longitude", long_name="longitude",
        units="degrees_east", coordinate="YC XC")),

some instances fixed where "coordinate" was spelled "coordinates".  May have no effect.
@rabernat
Copy link
Member

Thanks so much for this Ian!

Can you think of a way to test that this feature is working properly? For example, perhaps some check in this test?

def test_read_grid(all_mds_datadirs):
"""Make sure we read all the grid variables."""
dirname, expected = all_mds_datadirs
ds = xmitgcm.open_mdsdataset(
dirname, iters=None, read_grid=True,
geometry=expected['geometry'])

@rabernat rabernat mentioned this pull request Nov 12, 2020
6 tasks
@rabernat
Copy link
Member

Ian, can you help us understand what impact this PR has? I agree there is a typo, but I'm struggling to understand the consequences of that typo.

If this coordinates / coordinates entry actually has no effect, we should just remove it.

@ifenty
Copy link
Contributor Author

ifenty commented Nov 16, 2020

@rabernat Consequences are probably zero with respect to any calculations because when the dataset is saved to netcdf, xarray creates a 'coordinate' attribute for data variables: a 'space separated list of auxillary coordinates' as per CF conventions. It seems that xmitgcm is specifing its own coordinate list (e.g., ['XC YC'] or ['XG YG']) which xarray might use instead of making its own list. Because of the typo, the saved netcdf dataset could have a variables with both a 'coordinates' attributes (misspelled coordiante added by xmitgcm) and a 'coordinate' attribute added by xarray. I believe this 'coordinates' attribute is used in some plotting software, such as panoply. Seems like it's ok to keep the code in 'xmitgcm'.

@rabernat
Copy link
Member

Ok understood. I'd prefer to have some sort of test for this, but since it's a very minor change, we can merge this as is.

Thanks!

@rabernat rabernat merged commit 09b3e3f into MITgcm:master Nov 16, 2020
fraserwg pushed a commit to fraserwg/xmitgcm that referenced this pull request Nov 23, 2021
…ITgcm#236)

convention here seems to be

    XC=dict(dims=["j", "i"], attrs=dict(
        standard_name="longitude", long_name="longitude",
        units="degrees_east", coordinate="YC XC")),

some instances fixed where "coordinate" was spelled "coordinates".  May have no effect.
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