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

partial fix for SAGA RGB composite tool #8418

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

gioman
Copy link
Contributor

@gioman gioman commented Nov 5, 2018

Description

The SAGA "RGB Composite" has wrong parameters names. Anyway this is not a complete fix:

this tool MUST output an "image", explanation here:

https://gis.stackexchange.com/a/301146

and this is the reason the tool output is generated as a special case here:

https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/saga/SagaAlgorithm.py#L324

but the problem (unresolved by this patch) is that now SAGA outputs only in its native format, while this export operations MUST generate a TIFF (for example), otherwise the output (now generated because of the correct parameters names) will not show the expected values/colors (in fact it won't even be a RGB raster).

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include fixes #11111 in the commit message next to the description
  • Commits which add new features are tagged with [FEATURE] in the commit message
  • Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
  • I have read the QGIS Coding Standards and this PR complies with them
  • This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit

@luipir
Copy link
Contributor

luipir commented Nov 5, 2018

but the problem (unresolved by this patch) is that now SAGA outputs only in its native format, while this export operations MUST generate a TIFF (for example), otherwise the output (now generated because of the correct parameters names) will not show the expected values/colors (in fact it won't even be a RGB raster).

It's a decision by design to avoid implicit transformation and leave the original format to the backend that are not designed to produce a different format. This can be solved cascading the algorithm to a converter (if available).

@luipir
Copy link
Contributor

luipir commented Nov 5, 2018

@gioman as usual, please add a test ;)

@gioman
Copy link
Contributor Author

gioman commented Nov 6, 2018

It's a decision by desing to avoid implicit transformation and leave the original format to the backend that are not designed to produce a different format. This can be solved cascading the algorithm to a converter (if available).

@luipir I'm aware that now SAGA outputs only in its native format. What I wanted to point in this PR is that in this specific case this strategy does not work. With the now fixed parameters names the tool will generate an output but this is wrong (is not the expected RGB raster). So this specific tool that is already handled as a special case must be tweaked to work.

In the SAGA command line here

https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/saga/SagaAlgorithm.py#L324

"filename" must be defined as (example) a TIF output.

I tried to add a + '.tif' here

https://github.com/qgis/QGIS/blob/master/python/plugins/processing/algs/saga/SagaAlgorithm.py#L321

but it still generate a SAGA raster.

@gioman
Copy link
Contributor Author

gioman commented Nov 6, 2018

@gioman as usual, please add a test ;)

@luipir waiting for a live workshop in Coruña ;)

@nyalldawson nyalldawson added this to the 3.6 milestone Nov 11, 2018
@stale
Copy link

stale bot commented Nov 25, 2018

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 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

    • add a description of workflows which this pull request fixes

    • add screenshots if applicable

  • 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 a week.

@stale stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 25, 2018
@stale
Copy link

stale bot commented Dec 2, 2018

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

@stale stale bot closed this Dec 2, 2018
@luipir luipir reopened this Dec 2, 2018
@stale stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 2, 2018
@luipir
Copy link
Contributor

luipir commented Dec 2, 2018

@nyalldawson ok to merge also without test? for me it's ok... waiting for a processing test crash course ;)

@nyalldawson
Copy link
Collaborator

@luipir my understanding is that this is only a partial fix and needs follow up work to fix the actual issue. @gioman can you confirm?

@luipir
Copy link
Contributor

luipir commented Dec 2, 2018

well, seems that at least this PR is necessary to generate correct values... other aspect is that these values are in a manageable format... btw wait for @gioman opinion

@nyalldawson nyalldawson merged commit 25bfca4 into qgis:master Dec 4, 2018
@nyalldawson
Copy link
Collaborator

Fix inbound

@nyalldawson
Copy link
Collaborator

See #8597

@gioman
Copy link
Contributor Author

gioman commented Dec 6, 2018

@gioman can you confirm?

@nyalldawson sorry fot late reply, yes I confirm.

@ThirstyGeo
Copy link

This issue is still present in QGIS version 3.4.3-Madeira - should the fix have rolled out in the last QGIS update?

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

4 participants