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

added support for source data with multiple time dimensions #340

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Piyush-Ingale
Copy link
Contributor

@Piyush-Ingale Piyush-Ingale commented Jun 1, 2023

resolves #305
solution is to either create multiple tiff files or store data as bands. So user can specify using --tiff_config which dimensions to consider while generating tiff files. tiff_config will look like this
{"dims":["isobaricInhPa"]}
By default if time or step dimension is present in source data then it will create multiple tiff files for these dimensions.

Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

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

Is there any way that the logic could be a bit simpler?

weather_mv/README.md Outdated Show resolved Hide resolved
weather_mv/loader_pipeline/ee.py Outdated Show resolved Hide resolved
weather_mv/loader_pipeline/sinks.py Show resolved Hide resolved
weather_mv/loader_pipeline/ee.py Outdated Show resolved Hide resolved
weather_mv/loader_pipeline/sinks.py Outdated Show resolved Hide resolved
common_dims = set(tiff_config['dims']).intersection(set(ds.dims)) if tiff_config else []
dims_map['_'.join(common_dims)].append(ds)
mdl = []
for keys,dsl in dims_map.items():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what's going on here. in this loop. Can you add comments and/or break this up into smaller functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactored the code and also added comments

@alxmrs
Copy link
Collaborator

alxmrs commented Jun 12, 2023

@Piyush-Ingale: I recommend seeking @deepgabani8's advice (and review) on this PR. In my experience, he has found a few simpler ways to accomplish similar goals.

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.

weather-mv ee does not support multiple time dimesions
2 participants