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 Airocean projection (formerly Dymaxion) #4303

Merged
merged 42 commits into from
Jan 8, 2025
Merged

Conversation

plouvart
Copy link
Contributor

@plouvart plouvart commented Nov 1, 2024

  • Closes Add Dymaxion/Fuller Projection #232
  • Tests added
  • Added clear title that can be used to generate release notes
  • Fully documented, including updating docs/source/*.rst for new API

@plouvart plouvart changed the title Add Airocean projection (formerly Dymaxion) #232 Add Airocean projection (formerly Dymaxion) Nov 1, 2024
src/projections/airocean.cpp Outdated Show resolved Hide resolved
src/projections/airocean.cpp Outdated Show resolved Hide resolved
@plouvart
Copy link
Contributor Author

plouvart commented Nov 1, 2024

Seems like cppcheck is complaining about some unused variable, despite the fact that it is used next line.
Edit: just saw your fix above :)

@rouault rouault added this to the 9.6.0 milestone Nov 1, 2024
@rouault
Copy link
Member

rouault commented Nov 1, 2024

A few typos in the doc reported in https://github.com/OSGeo/PROJ/actions/runs/11632419978/job/32395481110?pr=4303 . For words which are legitimate and must be allowed, you can add them in docs/source/spelling_wordlist.txt

@plouvart
Copy link
Contributor Author

plouvart commented Nov 1, 2024

Thanks for the fix for the out of projection domain error

test/gie/builtins.gie Outdated Show resolved Hide resolved
@plouvart
Copy link
Contributor Author

a few extra tests to check the behavior of +orient would be nice

Tests seem to fail in builtins, but I see no error about airocean specifically. What could be the reason?

test/gie/builtins.gie Outdated Show resolved Hide resolved
// By default the resulting orientation of the projection is vertical
// the following transforms are used to alter the projection data
// so that the resulting orientation is horizontal instead
constexpr double orient_horizontal_trans[4][4] = {{0.0, -1.0, 0.0, 36843762.068421006}, {1.0, 0.0, 0.0, 0.0}, {0.0, 0.0, 1.0, 0.0}, {0.0, 0.0, 0.0, 1.0}};
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the value 36843762.068421006 specific to the use of the GRS80 ellipsoid ?
And I suspect that P->left and P->right shouldn't be overridden, and P->a not used directly, to let the generic code in src/fwd.cpp and src/inv.cpp do the scaling from the unit ellipsoid to the target one.

Copy link
Member

Choose a reason for hiding this comment

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

@plouvart gentle ping w.r.t my above question

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @rouault
Sorry for the lack of response over the last few week's. Lots of personal and work related obligations.
I will have time for a closer look at the issue this week end.

Copy link
Contributor Author

@plouvart plouvart Dec 9, 2024

Choose a reason for hiding this comment

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

@rouault
I remember that I got the inspiration for the P->right, P->left and P->a lines from the s2 projection file
I didn't know that src/fwd and src/inv does that for us automatically. I can remove those lines.

About the 36843762.068421006 value, it is related to the final scale of the airocean space whose dimensions stays constant whatever ellipsoid model we use (since it all gets normalized to the unit sphere in the end anyway).
Unless I'm mistaken?

Copy link
Member

Choose a reason for hiding this comment

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

About the 36843762.068421006 value, it is related to the final scale of the airocean space whose dimensions stays constant whatever ellipsoid model we use (since it all gets normalized to the unit sphere in the end anyway).

sounds surprising to me that the extent of the projection is constant and not dependent on the ellipsoid semi-major axis, but I don't know anything about that projection. I guess a could way to check that would be to add round-trip tests with the unit ellipsoid

Copy link
Member

Choose a reason for hiding this comment

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

Where in the repo should I put it?

I'd say in scripts/

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 added the notebook (with markdown comments), as well as a mention in the projection source file.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to convert the notebook to a pure python script? While notebooks are fairly common these days it's definitely not something everyone is familiar with (me included) and just browsing the file without opening it in the intended way is quite overwhelming. And again, this might be useful again in a couple of decades when notebooks will have been replaced by whatever is in at the time.
The image that is encoded within the notebook is likely very helpful in general and could be placed in the user documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbevers
I see your point. I will convert it to plain python when I have time tonight.

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 removed the notebook and added a script that takes no arguments and generates the 2 plot images and prints the C code on the standard output.
The only required import for printing the C code is the numpy library.

],
]
)
ico_20_centers = np.array(
Copy link
Member

Choose a reason for hiding this comment

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

Where do the below numeric values come from ?

Copy link
Member

Choose a reason for hiding this comment

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

This appears to be unused, so could be removed.

…osahedron vertices used for the airocean projection
@rouault
Copy link
Member

rouault commented Jan 6, 2025

Looks good to me. At others, any remaining comment before merging?

scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
Copy link
Member

@mwtoews mwtoews left a comment

Choose a reason for hiding this comment

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

Great work! I have a few other suggestions...

Some of the parameters very close to zero or one could do with a wee bit of rounding. Here is a def that could be put near the top and used in a few places:

def zap_zero_or_one(vals, eps=1e-12, verbose=False):
    """Return copy of array with values very close to zero and one set exactly."""
    vals = vals.copy()
    sel = (np.abs(vals) < eps) & (vals != 0.0)
    if sel.any():
        vals[sel] = 0.0
        if verbose:
            print(f"adjusted {sel.sum()} zeros")
    ones = np.abs(np.abs(vals) - 1.0)
    sel = (ones < eps) & (ones != 0.0)
    if sel.any():
        vals[sel] = np.sign(vals[sel])
        if verbose:
            print(f"adjusted {sel.sum()} ones")
    return vals

scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
scripts/build_airocean_parameters.py Outdated Show resolved Hide resolved
@rouault rouault merged commit ca80a5d into OSGeo:master Jan 8, 2025
2 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.

Add Dymaxion/Fuller Projection
5 participants