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.

Sign up for GitHub

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

Jump to bottom

gdal::VectorX Prototype #10744

Merged
merged 2 commits into from
Nov 13, 2024
Merged

gdal::VectorX Prototype #10744

merged 2 commits into from
Nov 13, 2024

Conversation

jjimenezshaw
Copy link
Contributor

@jjimenezshaw jjimenezshaw commented Sep 7, 2024 •
edited
Loading

What does this PR do?

During my work on the file alg/gdal_interpolateatpoint.cpp, I saw that there was a repetitive pattern: doing everything on X and Y. Both as integer to manage the pixel, and as double to use the coordinates in the image.

I missed a functionality where x,y (or even x,y,z) could be used as an object, and do some basic operations on that, like subtract another point, multiply by a factor, etc. Something I am used to with Eigen library.

Once that development was finished I tried something, to see if it makes sense. This is it.

The main feature is the template class VectorX. Both the size and type are templated. You can make it double, int, even complex. Size can be any, but I think the most common would be 2, maybe 3. Four predefined types are included: Vector2i, Vector3i, Vector2d, Vector3d

This PR is using it only in alg/gdal_interpolateatpoint.cpp.

The idea is to use it internally. Not in any public API.

It should make the code more readable, and I hope it will help to not forget the Y or the X during developments.

What do you think? Is it useful? In case it is included, I don't expect a massive migration, but just start using it when it is convenient.

What are related issues/pull requests?

#10461, #10506, #10570

Tasklist

  • Make sure code is correctly formatted (cf pre-commit configuration)
  • Add test case(s)
  • Add documentation
  • Updated Python API documentation (swig/include/python/docs/)
  • Review
  • Adjust for comments
  • All CI builds and checks have passed

@coveralls
Copy link
Collaborator

coveralls commented Sep 7, 2024 •
edited
Loading

Coverage Status

coverage: 73.7% (+0.01%) from 73.69%
when pulling ba20ede on jjimenezshaw:prototype-pixel
into fbaa99c on OSGeo:master.

rouault
rouault reviewed Sep 7, 2024
View reviewed changes
Copy link
Member

@rouault rouault left a comment

Choose a reason for hiding this comment

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

For testing, I assume you should be able to just include this .h file in a autotest/cpp/test_gdal_point_templ.cpp file added to gdal_unit_test

alg/gdal_point_templ.h Outdated Show resolved Hide resolved
alg/gdal_point_templ.h Outdated Show resolved Hide resolved
alg/gdal_point_templ.h Outdated Show resolved Hide resolved
@abellgithub
Copy link
Contributor

What is "raw" about this point? Are there use cases other than the one here?

@jjimenezshaw
Copy link
Contributor Author

What is "raw" about this point?

TBH I do not remember. Maybe the point in the image coordinates, not in the CRS. But it should not be restricted to it. I am not happy with the name of the class, as I said in the PR description. I didn't call it directly Point because it could be too generic; or Vector that is even more generic. Maybe just Point is a better idea. Suggestions are welcome.

Are there use cases other than the one here?

I hope so. Any algorithm that uses the coordinates of a point in the raster; or even in a geometry. This is a prototype to see if it makes sense (I didn't want to the class without any usecase). Once @rouault said it could be useful, I will improve it, with better documentation and tests.
My intention is not to use it everywhere now. Just when the developer finds it useful. I find more manageable a point as an object, and not having x and y separated in two variables.

@dbaston this is the PR I mentioned in the meeting.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 5, 2024

The GDAL project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 28 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

  • that all unit tests are passing

  • that all comments by reviewers have been addressed

  • that there is enough information for reviewers, in particular link
    to any issues which this pull request fixes

  • that you have written unit tests where possible
    In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
    If there is no further activity on this pull request, it will be closed in 2 weeks.

@github-actions github-actions bot added the stale label Nov 5, 2024
@stale stale bot removed the stale label Nov 10, 2024
rouault
rouault reviewed Nov 10, 2024
View reviewed changes
alg/gdal_vectorx_templ.h Outdated Show resolved Hide resolved
@jjimenezshaw jjimenezshaw changed the title RawPoint Prototype gdal::VectorX Prototype Nov 11, 2024
@jjimenezshaw jjimenezshaw marked this pull request as ready for review November 11, 2024 17:37
rouault
rouault reviewed Nov 12, 2024
View reviewed changes
alg/gdal_interpolateatpoint.cpp Outdated
@@ -17,6 +17,8 @@

#include "gdalresamplingkernels.h"

#include "gdal_vectorx_templ.h"
Copy link
Member

Choose a reason for hiding this comment

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

what about shortening that to "gdal_vectorx.h" ? The template thing is just an implementation detail that a user using gdal::Vector2i wouldn't notice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

BTW, if there is a better location for this file than the folder alg, I can move it.

Copy link
Member

Choose a reason for hiding this comment

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

Would probably be better in gcore/

@abellgithub
Copy link
Contributor

If you're wanting this, why not just use Eigen or similar?

@jjimenezshaw
Copy link
Contributor Author

If you're wanting this, why not just use Eigen or similar?

Adding a dependency like Eigen for such a small thing I think is not worth it.

@rouault
Copy link
Member

rouault commented Nov 12, 2024

Adding a dependency like Eigen for such a small thing I think is not worth it.

yes, the aim is not to do advanced linear algebra, but just to transport simple coordinate tuples. So perhaps that should be gdal::Tuple ?

@jjimenezshaw
Copy link
Contributor Author

So perhaps that should be gdal::Tuple ?

It is more than just a tuple. You can add an scalar to the vector, adding it to all the terms. Or multiply by a scalar. Or compute the norm.

@rouault
Copy link
Member

rouault commented Nov 12, 2024

It is more than just a tuple.

ok, gdal::VectorX is fine then. (ignore the Alpine failures. They are linked to package upgrades on their side)

jjimenezshaw
jjimenezshaw commented Nov 12, 2024
View reviewed changes
alg/gdal_vectorx.h Outdated
#ifndef GDAL_VECTORX_H_INCLUDED
#define GDAL_VECTORX_H_INCLUDED

/*! @cond Doxygen_Suppress */
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 think this is a leftover of a copy-paste. Should this class be in Doxygen? I think so.

Copy link
Member

Choose a reason for hiding this comment

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

As we don't install the file, this is debatable. There should be at least one warning in the class introduction that the file is for GDAL internal use for now

@rouault
Copy link
Member

rouault commented Nov 12, 2024

@jjimenezshaw anything left on your side before merging?

@jjimenezshaw
Copy link
Contributor Author

@jjimenezshaw anything left on your side before merging?

I will add tomorrow the comment about the usage

@jjimenezshaw
Copy link
Contributor Author

@jjimenezshaw anything left on your side before merging?

I think I am fine.

@rouault rouault added this to the 3.11.0 milestone Nov 13, 2024
@rouault rouault merged commit 57dc495 into OSGeo:master Nov 13, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rouault rouault rouault left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jjimenezshaw @coveralls @abellgithub @rouault