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

WeatherXds: time_weather #335

Closed
kettenis opened this issue Nov 25, 2024 · 4 comments · Fixed by #351
Closed

WeatherXds: time_weather #335

kettenis opened this issue Nov 25, 2024 · 4 comments · Fixed by #351
Assignees
Labels
bug Something isn't working enhancement New feature or request MSv4 Review

Comments

@kettenis
Copy link
Contributor

Description talks about time_cal instead of time_weather.

Also in PhaseCalibrationXds we have time_phase_cal. Maybe it would make sense to call all of these time_cal?

@Jan-Willem
Copy link
Member

WeatherXds has been updated to refer to time_weather only.

Convention time/time_* is used to indicate whether the time axis has been interpolated to the correlated_xds's time axis. Since these are separate datasets we choose to use distinct names for each.

@FedeMPouzols
Copy link
Collaborator

Oh yes, sorry the wrong time_cal in WeatherXds was an error in the schema that is now fixed, but I forgot to link the PR to this issue.

Given that the xds specific time_* of PhaseCalibrationXds is time_phase_cal, perhaps we could use time_system_cal for SystemCalibrationXds (rather than the generic time_cal which could be confused as referring to other cals)?

A similar consideration would apply to frequency_cal, although that one is only used in once sub-xds: SystemCalibrationXds. Use frequency_system_cal to be fully explicit?

@FedeMPouzols FedeMPouzols self-assigned this Jan 10, 2025
@FedeMPouzols
Copy link
Collaborator

The main issue reported here (time_cal=>time_weather in WeatherXds) was fixed already in #345.

What I'd proposed based on the comments above is, to make coordinate names more explicit and specific to every xds:

  • time_cal => time_system_cal in SystemCalibrationXds
  • frequency_cal => frequency_system_cal, also in SystemCalibrationXds.

Please let me know if this sounds fine. Otherwise there is not much more to do in this issue as far as I can see.

@FedeMPouzols FedeMPouzols added bug Something isn't working enhancement New feature or request labels Jan 15, 2025
@Jan-Willem
Copy link
Member

@FedeMPouzols, I agree that we should make the names more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request MSv4 Review
Projects
None yet
3 participants