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

Calculation of Transversal Mercator Projection for glacier coordinates #68

Merged
merged 5 commits into from
Feb 17, 2025

Conversation

facusapienza21
Copy link
Member

@albangossard @JordiBolibar I worked a bit more on the solution of #67

I am still not sure the computed projections are 100% correct, but this is a first step forward in having actual latitude/longitude grid. Please take a look at the code and let me know if you have any suggestions!

Please don't accept this PR yet, since I will probably try to polish a few pieces in the code soon. But at least you can have a flavor of how this would look like.

@facusapienza21
Copy link
Member Author

And thank you @albangossard for suggesting using CoordRefSystems instead of Geodesy: it is much more easy and intuitive to use, althought I spend quite some hours understanding how Mercator projections work... all this mess when the Earth is flat...

Copy link
Member

@albangossard albangossard left a comment

Choose a reason for hiding this comment

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

Hey @facusapienza21, thank you for that work! Computing the projections with CoordRefSystems looks pretty straightforward. I like the idea of having the coordinates in a global format, this will make the post-processing easier to plot and to interface with external data sources.

Comment on lines +134 to +135
latitudes = map(x -> x.lat.val, transform.(Ref(mean(easting)), northing))
longitudes = map(x -> x.lon.val, transform.(easting, Ref(mean(northing))))
Copy link
Member

Choose a reason for hiding this comment

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

I have the feeling that these averages are a bit touchy. From what I understand this is to compute the central point. Is the grid always uniform? Otherwise the projection might be incorrect in some situations. At the minimum we could check that the spacing is uniform, don't you think?

@facusapienza21
Copy link
Member Author

Following on @albangossard point about the uniform grid points:

I have the feeling that these averages are a bit touchy. From what I understand this is to compute the central point. Is the grid always uniform? Otherwise the projection might be incorrect in some situations. At the minimum we could check that the spacing is uniform, don't you think?

Yes, I thought about this as I was doing it... It is true that the glacier won't fix exactly in a square grid, but for mountain glaciers this discrepancy is minimal (even smaller than with usual projections, since the UTM projection is done with a custom longitude centered at the glacier). This discrepancies are already present in the dataset since the glacier products are reported in northing and easting, so grinding here has already been made. The conversion to latitude and longitude can introduce some small discrepancies, but it is rather minimal I will say.

This differences are more considerate in high latitudes. I added a warning message in the new version of the code to report to be careful with this when the latitude is near polar regions (~80 degrees ref).

@facusapienza21
Copy link
Member Author

Notice that I also added one more @testset so now all test are executed, instead of running the test until the first one fails.

I am not sure how the files for the glacier construct tests where created, but maybe we need to update them?

@albangossard
Copy link
Member

Yes, I thought about this as I was doing it... It is true that the glacier won't fix exactly in a square grid, but for mountain glaciers this discrepancy is minimal (even smaller than with usual projections, since the UTM projection is done with a custom longitude centered at the glacier). This discrepancies are already present in the dataset since the glacier products are reported in northing and easting, so grinding here has already been made. The conversion to latitude and longitude can introduce some small discrepancies, but it is rather minimal I will say.

This differences are more considerate in high latitudes. I added a warning message in the new version of the code to report to be careful with this when the latitude is near polar regions (~80 degrees ref).

Thank you for this complete explanation!
As long as the user is warned about potential projection error I'm ok with the current implementation.

I am not sure how the files for the glacier construct tests where created, but maybe we need to update them?

I double checked the content of the jld2 files when creating them but as S_coords has been replaced by Coords you'll probably need to update them. I would say if you're sure about the implementation of the projection, then you can safely overwrite them!

@facusapienza21
Copy link
Member Author

I updated the files but the errors don't pass in CI. I had run the tests in local and they work perfectly well, so I am not sure the source of the issue. I noticed there are some comments regarding tests in CI in Sleipnir, so maybe this is the issue?

Let me know if you can fix this on your side. I am going to be offline until next Tuesday, so feel free to merge the PR and we figure this out later.

@albangossard albangossard merged commit ec4d542 into ODINN-SciML:main Feb 17, 2025
0 of 2 checks passed
@albangossard
Copy link
Member

I updated the files but the errors don't pass in CI. I had run the tests in local and they work perfectly well, so I am not sure the source of the issue. I noticed there are some comments regarding tests in CI in Sleipnir, so maybe this is the issue?

Let me know if you can fix this on your side. I am going to be offline until next Tuesday, so feel free to merge the PR and we figure this out later.

After having updated the jld2 files in #69, the tests finally passed both locally and in the CI. I'm not sure to understand why. Anyway it's working now!

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