schism-dev / schism

Semi-implicit Cross-scale Hydroscience Integrated System Model (SCHISM)
http://ccrm.vims.edu/schismweb/
Apache License 2.0
78 stars 84 forks source link

Added time variable units, standard_name and axis attributes #76

Closed platipodium closed 1 year ago

platipodium commented 1 year ago

I will be making more changes improving UGRID standard compatibility in this branch. See http://cfconventions.org/Data/cf-conventions/cf-conventions-1.9/cf-conventions.html#coordinate-system for reference.

So far, the following additions have been made in scribe_io.F90

Is there anything that speaks against these additions?

Yet not added are

Current bugs

platipodium commented 1 year ago
josephzhang8 commented 1 year ago

@platipodium:

Thx for your work. None of my scripts use that info so in general I'm fine adding UGRID decor there.

My comments: 1) x,y units are not always lon, lat so this is misleading; 2) I think you can easily accommodate the utc_start or start_hour being real in those write statements? 3) Calculation of side/elem centers is wrong across lon jumps (that's why mu scripts calculate these separately) 4) The fact that we have split the outputs into multiple pieces already violates UGRID, so I really don't see the point of overloading 3D outputs with metadata and static arrays (file size). It's more than sufficient to have this in out2d*.nc.

platipodium commented 1 year ago

None of my scripts use that info so in general I'm fine adding UGRID decor there.

Our visualisation pipeline requires this. Obligatory, not decor.

platipodium commented 1 year ago

The fact that we have split the outputs into multiple pieces already violates UGRID, so I really don't see the point of overloading 3D outputs with metadata and static arrays (file size). It's more than sufficient to have this in out2d*.nc.

platipodium commented 1 year ago

x,y units are not always lon, lat so this is misleading;

This has been taken care of by #d90df816d

platipodium commented 1 year ago

Calculation of side/elem centers is wrong across lon jumps (that's why my scripts calculate these separately)

@josephzhang8 please fix the calculation or point me to a correct calculation

josephzhang8 commented 1 year ago

Thx, Carsten. I like your suggestion of iof_ugrid as a compromise (even though adding deco into 3D outputs results in small increase for each, collectively they add up quickly).

Correct calculation of side/elem centers requires passing 3D coordinates (xnd,ynd,znd) to scribes. I'll do that in the future and for the time being, let's add a reminder there. Thx.

-Joseph

Y. Joseph Zhang Web: schism.wiki Office: 804 684 7466

From: Carsten Lemmen @.> Sent: Wednesday, August 31, 2022 11:54 AM To: schism-dev/schism @.> Cc: Y. Joseph Zhang @.>; Mention @.> Subject: Re: [schism-dev/schism] Added time variable units, standard_name and axis attributes (PR #76)

[EXTERNAL to VIMS received message]

Calculation of side/elem centers is wrong across lon jumps (that's why my scripts calculate these separately)

@josephzhang8https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fjosephzhang8&data=05%7C01%7Cyjzhang%40vims.edu%7C57943640ad354c8281f008da8b690838%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637975580468987399%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2Fk6rZBg%2BsmP7lpTPRJEzXepPg99CrlE0BmeA65uPy2U%3D&reserved=0 please fix the calculation or point me to a correct calculation

- Reply to this email directly, view it on GitHubhttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fschism-dev%2Fschism%2Fpull%2F76%23issuecomment-1233122072&data=05%7C01%7Cyjzhang%40vims.edu%7C57943640ad354c8281f008da8b690838%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637975580468987399%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lHAAOI9v2CJUsc6DN1alhj8qu0MtzZ0M8icdv8zQtSI%3D&reserved=0, or unsubscribehttps://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAFBKNZ5UMCNIM72JEUCBTS3V355ZZANCNFSM56Z3PWPQ&data=05%7C01%7Cyjzhang%40vims.edu%7C57943640ad354c8281f008da8b690838%7C8cbcddd9588d4e3b9c1e2367dbdf1740%7C0%7C0%7C637975580468987399%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NmHM4pm5NPasfuD4AnTI0XKNd%2ByKmHVslolkYHFwIoE%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.**@.>>

josephzhang8 commented 1 year ago

@Carsten: the new code is good enough and I'll go ahead and merge to master. We can continue to work on it afterward. With some many changes recently from different groups, it's better work directly in master for this type of changes that have minimal impact on others. Thx.

josephzhang8 commented 1 year ago

Merged with git cmd.