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

Making attribute names more precise: obsdim->obs_dimname, timedim->time_varname #142

Merged
merged 4 commits into from
Nov 13, 2024

Conversation

knutfrode
Copy link
Contributor

So far mainly a mechanical search & replace
Thus some comments are still inconsistent, e.g. refering to time_varname as time dimension.

Also remains to add an attribute trajectory_dimname, but can perhaps be made in a subsequent pull request. So far this is hardcoded as "trajectory" at some places.

@gauteh
Copy link
Member

gauteh commented Nov 13, 2024

Should we use dim and not dimname in the public API? I think that is what xarray is doing: https://docs.xarray.dev/en/latest/generated/xarray.Dataset.reduce.html#xarray.Dataset.reduce

@knutfrode
Copy link
Contributor Author

Hm, ok yes, we should aim to be consistent with Xarray.
Shall I then replace all obs_dimname with obs_dim ?

But time_varname I guess is ok, as time_var could be interpreted both as the name of the time variable or as the variable itself. For dimension we do not have this dual possibility.

@gauteh
Copy link
Member

gauteh commented Nov 13, 2024

Yes, I think this is the best compromise?

logger.debug('Detected un-structured (2D) trajectory dataset')
ocls = Traj2d

else:
raise ValueError(
f'Time dimension has shape greater than 2: {ds["timedim"].shape}'
f'Time dimension has shape greater than 2: {ds["time_varname"].shape}'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This an example of why old names were not precise enough.
Thus Time dimension has shape greater than 2 should now be changed to the more precise Time variable has more than 2 dimensions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, also this probably needs to be: ds[self.time_varname] without the internal quotes.

@knutfrode knutfrode merged commit 391f3e2 into OpenDrift:main Nov 13, 2024
12 checks passed
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