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

Implement normalizeForVisualization for DerivedProjected #3477

Merged

Conversation

jjimenezshaw
Copy link
Contributor

@jjimenezshaw jjimenezshaw commented Nov 18, 2022

  • Tests added

Inspecting the code I found that the functions normalizeForVisualization and applyAxisOrderReversal are not implemented for DerivedProjected.

@jjimenezshaw
Copy link
Contributor Author

jjimenezshaw commented Nov 18, 2022

Tests are pending. I do not know if I should implement them in test_crs.cpp or test_operation.cpp. Any preference?

I have tested it locally with projinfo using the undocumented option --normalize-axis-order. Is it not documented in purpose?

@jjimenezshaw
Copy link
Contributor Author

jjimenezshaw commented Nov 18, 2022

I would prefer to include the variable definition inside the if scope, like

    if (const DerivedProjectedCRS *derivedProjCRS =
            dynamic_cast<const DerivedProjectedCRS *>(this)) {
        const auto &axisList = derivedProjCRS->coordinateSystem()->axisList();
        if (mustAxisOrderBeSwitchedForVisualizationInternal(axisList)) {
            return applyAxisOrderReversal(NORMALIZED_AXIS_ORDER_SUFFIX_STR);
        }
    }

But I have not done it for consistency with the previous. Should I change it in these two functions?
(I was almost using projCRS instead of derivedProjCRS, and it compiled)

@rouault
Copy link
Member

rouault commented Nov 18, 2022

in test_crs.cpp

that would be the better place. Strangely I don't find direct testing of CRS::normalizeForVisualization() on other CRS types. It must be currently done indirectly through operations that involve them, but direct unit testing would be better practice

Is it not documented in purpose?

looking at the comment of 88426c1 , yes apparently :-) Probably because I'm moderately comfortable with the concept of "normalized axis order" which isn't super well defined.

I would prefer to include the variable definition inside the if scope

yes please change other occurences if you wish

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.

None yet

2 participants