Consistent variable attributes for time coordinates in `Environment` and `Platform` groups

emiliom commented 2 years ago

This issue stems from PR #672, separating out improvements and harmonization of time variable attributes from the actual renaming of time dimensions and variables (in Environment, Platform and Platform/NMEA groups) that are the focus of that PR.

I will pull out relevant comments I made in that PR and place them in this issue.

Related to #656.

b-reyes commented 2 years ago

Small note: I think we should reconsider the comment for time1 too. After #673 has been implemented, time1 can correspond to more than just the GPS location (it will be all times associated with the NMEA datagram). As it currently stands, the comment attribute would only be correct for Platform.time1 (as stated, this will change after #673).

Perhaps the following comment for Platform.time1 and Platform/NMEA.time1 would be more appropriate: "Time coordinate corresponding to NMEA sensor data."

emiliom commented 2 years ago

`long_name` attribute

https://github.com/OSOceanAcoustics/echopype/blob/099b357061de006bea7710899bc78988bad13ecb/echopype/convert/set_groups_ek60.py#L329-L337

long_name (here, EK60) may need to be changed, otherwise it's identical (indistinguishable) to the one in time2. Maybe "Timestamps for environmental variables"? Though that would change the nature of the long_name from the data origin (datagram type) to, umm, its scope. The same probably applies for AZFP and EK80.

`comment` attribute

https://github.com/OSOceanAcoustics/echopype/blob/099b357061de006bea7710899bc78988bad13ecb/echopype/convert/set_groups_ek60.py#L319-L328

I think this EK60 comment attribute should be something like "Time coordinate corresponding to platform motion and orientation sensors.". See @leewujung's initial comment/description in #656, though I'm suggesting changing "position" to "orientation", since the former could also apply to GPS/time1.

BUT, I see that my use of orientation instead of position may conflict with the pre-existing long_name string, which refers to position datagrams. @leewujung may need to weigh in.

Same for AZFP: https://github.com/OSOceanAcoustics/echopype/blob/099b357061de006bea7710899bc78988bad13ecb/echopype/convert/set_groups_azfp.py#L129

and EK80: https://github.com/OSOceanAcoustics/echopype/blob/099b357061de006bea7710899bc78988bad13ecb/echopype/convert/set_groups_ek80.py#L357

b-reyes commented 2 years ago

Small note: I think we should reconsider the comment for time1 too. After #673 has been implemented, time1 can correspond to more than just the GPS location (it will be all times associated with the NMEA datagram). As it currently stands, the comment attribute would only be correct for Platform.time1 (as stated, this will change after #673).

Perhaps the following comment for Platform.time1 and Platform/NMEA.time1 would be more appropriate: "Time coordinate corresponding to NMEA sensor data."

In #673 @leewujung and myself came to the conclusion that we will NOT be making Platform.time1 and Platform/NMEA.time1 the same. However, the comment attribute is still not appropriate. I suggest the following change:

Platform/NMEA.time1.attrs["comment"] = "Time coordinate corresponding to NMEA sensor data."

leewujung commented 2 years ago

Platform/NMEA.time1.attrs["comment"] = "Time coordinate corresponding to NMEA sensor data."

I agree with this.

I think Platform.time1.attrs["comment"] should probably be "Time coordinate corresponding to NMEA position data." because those are derived from the same NMEA sensor, but we only select out the position (lat/lon) data.

leewujung commented 2 years ago

For Platform.time2.attrs["comment"]: @emiliom I am not sure what you are referring to in terms of pre-existing long_name conflicts. But I think "Time coordinate corresponding to platform motion and orientation sensors." is appropriate.

Note that these data are not from the GPS sensor. It is from the MRU.

emiliom commented 2 years ago

@leewujung I meant the long_name in time2, as seen in the code block above that statement: "Timestamps for position datagrams". Namely, if we change the comment attribute to "... platform motion and orientation ..." (instead of "... position ..."), maybe the long name should be changed too; but that may not be consistent with the datagram origin stated in long_name.

You suggestion works for me: "Time coordinate corresponding to platform motion and orientation sensors."

leewujung commented 2 years ago

Ok, so Platform.time2.attrs["comment"]="Time coordinate corresponding to platform motion and orientation sensors."

As for the long_name, these data are not from a position sensor, but a motion sensor, and they do not come from the NMEA datagram. In EK60 they come from the sample datagram. In EK80 they come from the MRU datagram, as noted in https://github.com/OSOceanAcoustics/echopype/issues/656#issue-1219104771 for the original mru_time.

So how about Platform.time2.attrs["long_name"] = "Timestamps for platform motion and orientation data." ?

emiliom commented 2 years ago

`time*` attribute changes in the `Platform` and `Platform/NMEA` groups

All variable attribute changes being recommended involve the Platform and Platform/NMEA groups only. We don't have any additional changes for attributes in Environment.

Platform.time1

Applies to EK60, EK80 (AZFP doesn't have this variable)
comment: "Time coordinate corresponding to NMEA position data"

Platform.time2

Applies to AZFP, EK60, EK80
long_name: "Timestamps for platform motion and orientation data"
comment: "Time coordinate corresponding to platform motion and orientation sensors."

Platform.time3

Applies to EK60, EK80 (AZFP doesn't have this variable)
long_name: "Timestamps for environmental data" ~~@leewujung can you confirm this?~~

Platform/NMEA.time1

Applies to EK60, EK80 (AZFP doesn't have this group)
comment: "Time coordinate corresponding to NMEA sensor data"

Note: no period at the end of the long_name string. For comment, I'd suggest being parsimonious (no period), but comment is an optional attribute intended to be free flowing, so I don't really care :stuck_out_tongue_winking_eye:

Updated: Added Platform/NMEA.time1 recommendation

leewujung commented 2 years ago

time3

Applies to EK60, EK80 (AZFP doesn't have this variable)

long_name: "Timestamps for environmental data" @leewujung can you confirm this?

I agree. This makes it general.

b-reyes commented 2 years ago

@emiliom thank you for your summary. Since Platform.time3 is the same as Environment.time1 for EK80, so we should also change the long_name for Environment.time1 to "Timestamps for environmental data". @leewujung for EK60, is Platform.time3 the same as Environment.time1? I know that in the code they use the same value, but I am unsure if we should also make their attributes consistent with one another.

leewujung commented 2 years ago

For EK60, yes Platform.time3 comes from the same source as Environment.time1, it is part of the sample datagram. I am not sure which attributes you are referring to, and honestly I have lost track on the distinction between long_name and comment and exactly what we have decided on.

@b-reyes : how about if you push up a PR with what you have at the moment, and we iterate there? This way the discussion is more coherent since we can see all the attributes at once. Thanks!

b-reyes commented 2 years ago

This has been addressed in #699.

OSOceanAcoustics / echopype