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

Update power presets following transformer tagging extension #447

Merged
merged 3 commits into from
Jul 11, 2022

Conversation

flacombe
Copy link
Contributor

@flacombe flacombe commented May 1, 2022

I propose theses changes following power transformer tagging extension.
It's my first PR here, I hope I didn't break anything and accurately followed contributing rules.

I didn't manage to introduce theses requirements in presets. Can someone help me please?

Closes #415

Copy link
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for this update! In general it looks good, but please find some comments inline and below:

Some voltage keys should also be discouraged. Is it possible to raise a warning regarding the following please ?

voltage=* in combination with power=transformer

The question is what should be done with the value of the tag if it is found? It surely should not be removed, because it can still be useful to determine the (proper) values for voltage:primary/secondary… In my opinion this is not a good fit for a validation in iD itself, but rather something for an external QA tool like osmose or maproulette.

[voltage-high=*](https://wiki.openstreetmap.org/wiki/Key:voltage-high)
[voltage-low=*](https://wiki.openstreetmap.org/wiki/Key:voltage-low)

These are included in the deprecation rules you provided.

Furthermore, voltage:primary, voltage:secondary, voltage:tertiary are valid with (already in presets) and should require transformer=* (not in validator) to be used.
https://wiki.openstreetmap.org/wiki/Tag:power%3Dtransformer#Voltage_tagging

Similar question as above: If iD can't provide is no good "upgrade" approach, and removing the data would add more harm than leaving it as it, it would be better to implement such a check in an external QA tool. Also, iD shows users the transformer field to fill in: What more do you expect?

transformer=* should only be used on nodes (it doesn't raise a warning on area currently), just like power=transformer (which accurately raises a warning when used on areas).

You mean when a transformer tag is present, but no power=transformer? I would estimate that that is quite rarely the case (because it would have had to be manually added directly in the tag editor by an "experienced" mapper)…

data/fields/transformer.json Outdated Show resolved Hide resolved
data/fields/transformer.json Outdated Show resolved Hide resolved
data/fields/windings/auto.json Outdated Show resolved Hide resolved
data/deprecated.json Show resolved Hide resolved
data/deprecated.json Outdated Show resolved Hide resolved
@flacombe
Copy link
Contributor Author

flacombe commented May 13, 2022

Hi @tyrasd and thank you for the comments.

Here are additional answers complementary with previous discussion upside.

The question is what should be done with the value of the tag if it is found? It surely should not be removed, because it can still be useful to determine the (proper) values for voltage:primary/secondary… In my opinion this is not a good fit for a validation in iD itself, but rather something for an external QA tool like osmose or maproulette.

Given problem is we don't know what voltage=* means in combination with transformers. It can be primary, secondary, tertiary or even a list of them. That's why it should not be used.
There are two different problems here

  • People can create a new osm feature with transformer in combination with voltage and they should be advised to use voltage:primary, secondary instead
  • An existing transformer is found with voltage=* and it's pretty impossible to guess if it's primary, secondary... just like a consumer won't be able to get any value from that.

Similar question as above: If iD can't provide is no good "upgrade" approach, and removing the data would add more harm than leaving it as it, it would be better to implement such a check in an external QA tool.

I'd say iD can put the greatest voltage:primary, voltage:secondary or voltage:tertiary in voltage=* and drop others, if possible.

Also, iD shows users the transformer field to fill in: What more do you expect?

It's not a matter of fields, but a matter of combinations.
voltage:primary, voltage:secondary... requires transformer=* to be used. Using them without transformer=* should raise a warning.
highway=street_lamp + voltage:primary=* is invalid and doesn't expect transformer=* to become valid.

You mean when a transformer tag is present, but no power=transformer? I would estimate that that is quite rarely the case (because it would have had to be manually added directly in the tag editor by an "experienced" mapper)…

That's right, we can keep that apart for this PR.
That would be useful to check what is added directly as well, not only experienced mappers directly add tags.

@flacombe
Copy link
Contributor Author

I'll resolve change requests in a few hours, wait for it.

@tyrasd tyrasd force-pushed the main branch 2 times, most recently from 9d3204d to 49f529e Compare June 22, 2022 16:19
@flacombe
Copy link
Contributor Author

flacombe commented Jul 7, 2022

Any news on this PR @tyrasd?

@tyrasd tyrasd merged commit de6fce4 into openstreetmap:main Jul 11, 2022
@tyrasd
Copy link
Member

tyrasd commented Jul 11, 2022

voltage:primary, voltage:secondary... requires transformer=* to be used. Using them without transformer=* should raise a warning.

I see. Can you please create a new feature request for this on the iD repo?

@flacombe
Copy link
Contributor Author

Thank you @tyrasd for review and final validation.

Be sure I will report the need to have dependency in tagging rules in iD repo.

Best regards

@flacombe flacombe deleted the power/transformers branch July 11, 2022 13:54
Comment on lines +7 to +12
"main": "Forwards power",
"distribution": "Feeds final consumers, installed outside substations",
"generator": "Steps-up voltage in power plants",
"converter": "Feeds power converters",
"phase_angle_regulator": "Regulates phase angle",
"auxiliary": "Feeds internal systems in substations",
Copy link
Contributor

Choose a reason for hiding this comment

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

Were the previous strings incorrect? The new strings can end up being so lengthy in translation that they ideally would be displayed as tooltips, though I’m unsure if schema-builder supports localizable combo option tooltips yet. For now, perhaps each option could start with the short name, like “Distribution: Feeds final consumers, installed outside substations”.

Copy link
Member

Choose a reason for hiding this comment

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

They were probably not incorrect, but I thought that having an almost 1:1 copy of the raw tag value isn't a big help for mappers (otherwise this field could have been a regular combo field), that's why I asked for more descriptive labels.

👍 for adding the tag values as part of the labels, I've tweaked it in a33d777 – does that look better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tagging changes for power transformers
3 participants