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

comparison processes read 4-digit values as EPSG, change them wrongfully #157

Closed
jonathom opened this issue Jun 18, 2021 · 6 comments
Closed
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jonathom
Copy link
Member

Consider the following to reproduce: Web Editor -> open a process lt, gte or the likes, at y parameter select type number and enter 4000, click save, open the process again, now type is EPSG and the first in the selection list is EPSG:2000, click save, now the value for y is 2000. As far as I can tell only 4 digit values and processes lt, lte, gte and gt are affected, didn't test all combinations though, i.e. only had a look at add and divide to confirm this is not affecting other math processes. (All while connected to VITO backend).

@m-mohr
Copy link
Member

m-mohr commented Jun 18, 2021

Yeah, the issue that another data type is "chosen" is not really possible to fix. It also applies to other data types. The issue is that just from the value (e.g. 4000) I can't detect which "visualization" to choose. I should probably fall back to the "native" one (e.g. number instead of EPSG) by default, but it will never be "exact". Then there will be cases where an EPSG code is identified just as a number...

What I can surely fix that EPSG:4000 is chosen (if it exists) instead of 2000...

Related: #80, #43

@m-mohr m-mohr added this to the v0.8.0 milestone Jun 18, 2021
@m-mohr m-mohr added the bug Something isn't working label Jun 18, 2021
@m-mohr m-mohr self-assigned this Jun 18, 2021
@jonathom
Copy link
Member Author

Why allow EPSGs at all?

@m-mohr
Copy link
Member

m-mohr commented Jun 18, 2021

Any data type is allowed so why restrict? I can't guess what the user wants to input...

@jonathom
Copy link
Member Author

True. Why is any data type allowed, given the documentation:

If any operand is not a number or temporal string (date, time or date-time), the process returns false.

? I don't wanna pry too much here, I just fail to imagine any use case.

Here's an idea: The menu gets a neutral option like "-Select type-". Whenever process options are opened, this "-Select type-" is shown in type field instead of EPSG or a best guess type. The value is represented from process graph, there is basic typing possible here I think (like show text field / bounding box depending on input). And when the value is changed it is just written to process graph as it was there before, if type is still set to that neutral option. This whole thing possibly needs some documentation in the process box then.

@m-mohr
Copy link
Member

m-mohr commented Jun 21, 2021

Okay, there were two issues here:

  1. EPSG selection had a bug that always made the select box reset to the first entry (2000) - this has been fixed.
  2. The detected data type was just taking the one that was alphabetically first if a conflict was detected. EPSG / CRS are both alphabetically sorted before "Number" so that's why the EPSG selector came up. I improved the detection in a way that it prefers native data types (here number) in favor of specific subtypes like EPSG in case of a conflict.
  3. I also did some refactoring and changing between strings and numbers keeps the values if numerical.

So the behavior should have improved in v0.7.7.

PS: Any data type is allowed to allow conditions to always return a result without throwing an error.

@m-mohr m-mohr closed this as completed Jun 21, 2021
@jonathom
Copy link
Member Author

Any data type is allowed to allow conditions to always return a result without throwing an error.

Thanks! That makes a lot of sense.

select box reset to the first entry

I realize now that with this fixed, the shown type is not as important anymore as values themselves aren't changed anymore. Other improvement sounds great though too! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants