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

Add cartesian projection #168

Merged
merged 8 commits into from
May 9, 2024
Merged

Conversation

lpilz
Copy link
Collaborator

@lpilz lpilz commented Apr 2, 2024

Change Summary

Added cartesian projection in grid treatment. This will cause the transformation (L64/66) to be an identity transformation. There could also be an argument for raising an error and just skipping the creation of the CRS and these coordinates as the idealized lat/lons probably don't correspond to actual coordinates.

Related issue number

Closes #163

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable

@lpilz lpilz requested a review from jthielen April 2, 2024 09:02
@lpilz
Copy link
Collaborator Author

lpilz commented Apr 2, 2024

I didn't create tests for this as we don't have any idealized files in xwrf-data. @MaxEtherington could you maybe share some?

@jthielen
Copy link
Collaborator

jthielen commented Apr 2, 2024

My instinct would be skipping grid/projection handling for this case rather than adding a possibly fictitious (since there isn't really a datum or projection to be referred to?) identity transformation, but having no real experience with idealized WRF output, I'd definitely defer to others with more experience. In any case, I'd agree that there should be at least some test of this (either way it is implemented) before going in.

@lpilz
Copy link
Collaborator Author

lpilz commented Apr 5, 2024

I see your point, @jthielen - was thinking the exact same thing. However, since we probably still want to create the coordinate values for the south_north/west_east and especially the *_stag dimensions I figured this would be the least invasive change. What do you think of the following?

@MaxEtherington what do you think of this implementation? And could you provide us with test files?

@MaxEtherington
Copy link

MaxEtherington commented Apr 6, 2024

Hi! Apologies for not getting back to you — I've been out on a geology mapping trip for the last week, and internet access has been correspondingly patchy.

Regarding implementation, skipping the creation of a CRS seems best; in the simulations I've run, ideal.exe simply rips the input lat/lon from namelist.input and applies the same values uniformly to every cell in the grid. That is to say, those 'coordinates' don't resemble anything real.

The wrfoutput files I've been working with are somewhat hefty (1.5–3.5 GB); how much would you like me to trim them down? How many timesteps would be useful? I'm admittedly fairly new to Github — what would be the best way to share them with you?

@lpilz
Copy link
Collaborator Author

lpilz commented Apr 11, 2024

Thanks for your reply. I'm fine with just not creating the CRS. In the end, if you're using idealized WRF runs, you should know what you are doing as a user.

Regarding the data, I would ask you to make a pull request in the xarray-contrib/xwrf-data repo adding the respective file with a sensible name like ideal.nc. I think just one timestep with one sensible variable (so including XLAT, TIMES and so on) is good, this should hopefully bring it down to a manageable size. Please consult the documentation on how to create a pull request. If you're stuck, I'm happy to help.

@lpilz
Copy link
Collaborator Author

lpilz commented May 2, 2024

@jthielen I added just one small test for the ideal file because the XLAT/XLONG coordinates don't seem to be sensible (they are all one value). If you have any further ideas what to test, let me know. Otherwise this would be good to go from my POV

@mgrover1
Copy link
Collaborator

mgrover1 commented May 8, 2024

@lpilz - I have a sample dataset we can pull in here - I wonder if we just make the projection optional as well, as we do with other postprocessing steps?

@mgrover1
Copy link
Collaborator

mgrover1 commented May 8, 2024

same thing here - let me know if you need a review @lpilz

@lpilz
Copy link
Collaborator Author

lpilz commented May 9, 2024

@lpilz - I have a sample dataset we can pull in here - I wonder if we just make the projection optional as well, as we do with other postprocessing steps?

Thanks! We already have a dataset in xwrf-data but if you think that there is something missing, we can of course add yours as well :)

Regarding the projection: That's probably a good idea, refactoring it to make it more intuitively useable was something that was on my mind anyways... Let's discuss this in a new issue.

same thing here - let me know if you need a review @lpilz

And if you could review, that would be great :)

@lpilz lpilz requested a review from mgrover1 May 9, 2024 12:13
@lpilz lpilz mentioned this pull request May 9, 2024
Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Looks good to me - thanks @lpilz !

@mgrover1 mgrover1 merged commit 59cbc05 into xarray-contrib:main May 9, 2024
6 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.

[MISC]: Cannot post-process cartesian wrfout data
4 participants