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

Revert rendering of healthcare key #3731

Merged
merged 1 commit into from
Apr 4, 2019

Conversation

jeisenbe
Copy link
Collaborator

Reverts #3498
Fixes #3639, #3606
Related to #3706, #3594, #3609

Can be reverted after a new version of #3644 is merged and database reloaded (probably some months in the future)

Changes proposed in this pull request:

  • Remove "healthcare" key from project.mml and amenity-points.mss
  • Removes rendering for features tagged with "healthcare" only (most healthcare features, which are also tagged with amenity=hospital, =clinic, =doctors, =pharmacy etc, will continue to render)

Explanation

Features mapped with the key "healthcare" are not currently loaded into the database as a polygon when mapped as a closed way. Fixing this requires database reload; see #3644 which was closed.

In the meantime, features mapped as closed ways are not be rendered when tagged with "healthcare" alone. This provides inconsistent mapper feedback, suggesting that features need to be double-tagged with "amenity" or mapped as a node to render.

Therefore, healthcare should not be rendered at this time. Ideally, the next database reload on the OSMF servers can take place sometime this year, and the healthcare tagging can be rendered again at that time.

Features mapped with the key "healthcare" are not currently loaded into the database as a polygon when mapped as a closed way. Fixing this requires database reload. In the meantime, features mapped as closed ways would not be rendered when tagged with "healthcare" alone
@Adamant36
Copy link
Contributor

Adamant36 commented Mar 27, 2019

In the meantime, features mapped as closed ways are not be rendered when tagged with "healthcare" alone. This provides inconsistent mapper feedback, suggesting that features need to be double-tagged with "amenity"

What's wrong with double tagging exactly? Last time I checked both tagging schemes are correct aren't they? More then removing the rendering of one, which kind of pushes people in the direction of over tagging the other, it should figured out which is the better tagging scheme and the none favored one should be phased out. I seem to remember there being a discussion about doing just that somewhere. It seems like a better option then encouraging people to not map healthcare as a closed way just because its not supported here yet, or whatever is going on here.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Mar 27, 2019 via email

@imagico
Copy link
Collaborator

imagico commented Mar 30, 2019

Frankly i am not sure what to make of this because i don't understand the reasoning behind the change this reverts. It to me looks like this was adding rendering of synonyms for existing hospital/dentist etc. tags - which is something we as a principle like to avoid here to support tagging consistency and avoid meaningless proliferation of tags.

In other words: This change makes sense to me because it reverts a change that does not seem to make sense. But i suspect i might be missing something.

@jeisenbe
Copy link
Collaborator Author

The original PR also added rendering (with red dots and a text label) for a number of healthcare-related features that have no alternate tagging:

          [feature = 'healthcare_alternative'],
	  [feature = 'healthcare_audiologist'],
	  [feature = 'healthcare_birthing_center'],
	  [feature = 'healthcare_blood_bank'],
	  [feature = 'healthcare_blood_donation'],
	  [feature = 'healthcare_dialysis'],
	  [feature = 'healthcare_laboratory'],
	  [feature = 'healthcare_midwife'],
	  [feature = 'healthcare_occupational_therapist'],
	  [feature = 'healthcare_optometrist'],
	  [feature = 'healthcare_physiotherapist'],
	  [feature = 'healthcare_podiatrist'],
	  [feature = 'healthcare_psychotherapist'],
	  [feature = 'healthcare_rehabilitation'],
	  [feature = 'healthcare_speech_therapist'],
	  [feature = 'healthcare_yes'] {

I think adding healthcare=yes was questionable, but I would support rendering the other features in the above list with a generic dot and name label, when it become possible.

However, because these features can be tagged on polygon geometires (closed ways and multipolygon relations), and at this time we cannot render these without a database reload, I have submitted this PR.

I believe this is reason enough to merge the change for the time being; we don't have to discuss the double-tagging issue at this time.

@jajajaneeneenee
Copy link

jajajaneeneenee commented Apr 2, 2019

I personally think this is not a very good idea. Why should mappers be frustrated who already used the new healthcare tagging scheme for POIs, which are not represented by the old scheme (which was one important reason for the newer, wider scheme, which is more powerful)? I mean those who tagged nodes, with healthcare=* (without amenity) which are rendered already now (except healthcare=counselling, which is in the wiki since March 2019, and I personally would like to render, too, because I think, it's an important part of a healthcare system). And this will disapear after this change for some month, for a year? Or perhaps longer? Only to avoid discussions and questions when it was used to map closed ways?

This could be made clearer in the Wiki ... and that it will take some month to fix this. If you remove the rendering for nodes (will have an effect only to those which are tagged with "healthcare" only, I know), you also will have to explain this in the wiki, I think ... and this may cause also some discussions and will take some time until every mapper did read it ... so what is the real advantage? Or is there any advantage for the mappers at all? Or is the only reason to remove an temporary inconsistency in the rendering – seen from a developer viewpoint? I don't get it completely ...

And I am not familiar enough with the problems of the database reload: but why don't put more energy in a quicker database reload (a PR for that?) to fix this problem completely much faster than some month or a year? But as I mentioned, this may be out of the scope of my knowledge (how difficult this may be). Only a question I had for myself ...

And I think, the new healthcare scheme makes a lot of sense an is quite well-thought-out ... it's a good advancement of the old "amenity scheme". And now we are in the transition phase with double tagging (no problem I think) ...but taggers should be encouraged to use (also) the new scheme. And the PR would perhaps cause the opposite. That's my opinion, or better: my feeling ...

(Quick view in the taginfo of "healthcare" – 2019-04-02: nodes=72 791, ways=36 004, so the ratio is 2:1 – very often combinded with amenity, of course – 77.56% – but there should be > 20.000 without amenity and perhaps around 14000 nodes WITHOUT amenity based on the 2:1 ratio – I didn't check it completely = potential of 14000 frustrations? I know ... that's only an abstract and rough calculation ...)

Only wanted to add some thoughts ... and I'm coming more from the mapper side (as you may already have noticed), but I also have some developer backgrounds since the 80's/90's ... so I decided to write this comment.

@Adamant36
Copy link
Contributor

Adamant36 commented Apr 2, 2019

why don't put more energy in a quicker database reload (a PR for that?) to fix this problem completely much faster than some month or a year?

Unfortunately, it's not that easy. From my understanding it's not really done alone, and gets dealt with when new LTS versions are Ubuntu are deployed. Along with other things, like newer stable versions the back end site stuff. Which usually happens once a year, usually in October if I remember correctly. There was talk about doing it every six months, but there just enough stuff to implement on that cycle most of the time to make it worth doing that frequently.

My personal opinion on this is that it might have been a good thing to do a while back, but now that it's almost the middle of the year and it will only be like 6 months until the reload, I don't think it's worth the temporary gain to implement. Given the other problems that will come along with it. Like confusing people as to why rendering a perfectly valid tagging scheme was removed, just to be implemented again in a few months. As @jajajaneeneenee says also, tagging both along side each other is helping it to get used more widely. Not rendering it might screw that up and cause the older tagging scheme to gain ground it shouldn't. Plus, the whole thing is a really minor issue in the first place in my opinion. It's definitely not on the top of the list of rendering problems. I've really only seen a few complaints about it. There will probably be more issues opened if its removed then there currently are about whatever problems it causes in the short term.

@imagico
Copy link
Collaborator

imagico commented Apr 2, 2019

I believe this is reason enough to merge the change for the time being; we don't have to discuss the double-tagging issue at this time.

Fine by me - i just wanted to make sure i did not miss any non-obvious reason for introducing rendering of synonyms for established tags here.

Regarding database reload - this does not depend on any external software version conditions - though a system upgrade would of course be a logical choice for doing a database reload. Anyone who is interested in making this happen sooner can help doing that by working on implementing this change - to be maintained in a separate branch until it can be integrated as discussed in #3611.

And keep in mind tagging discussion is doubly off-topic here (in this PR and in this repository in general).

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

I am fine with this because the described problem is definitely something that should have prevented merging of #3498. But i also view the rendering of synonyms for the same type of feature very critically and would like us to revisit this decision (with several different possible outcomes this could lead to). Therefore i am not really able to look at this purely from the perspective of the effects of inconsistent rendering of closed ways and would therefore like another opinion on the matter.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Apr 2, 2019

This problem has no good solution for me, unfortunately. I believe if there is now someone to take care of the full upgrade, we could merge this for now, otherwise it's better to leave the current state to not create additional confusion and code mangling.

@matthijsmelissen
Copy link
Collaborator

the described problem is definitely something that should have prevented merging of #3498

+1

@imagico
Copy link
Collaborator

imagico commented Apr 2, 2019

@kocio-pl - i don't quite understand your statement, could you re-formulate to make it clearer?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Apr 2, 2019

If somebody plans to take care of the code to be properly rendered in all the cases (in few months probably), we could revert now. If nobody is ready now to take care of that process, I think we should not revert it.

I hope this is more clear now.

@imagico
Copy link
Collaborator

imagico commented Apr 2, 2019

That's clearer now, thanks.

I think preparing a change reverting this PR later would not pose significant technical difficulty. But i also think asking anyone here for a long term assurance that they will prepare a change at an undefined time in the future is not something we can or should ask in a volunteer project.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Apr 3, 2019

Nobody is forced to do anything. I just vote with my trust on somebody willing to provide a solid solution, because it would overweight other problems and risks I'm still afraid of (expressed by @jajajaneeneenee and @Adamant36).

@pnorman pnorman merged commit 26c5e69 into gravitystorm:master Apr 4, 2019
@pnorman
Copy link
Collaborator

pnorman commented Apr 4, 2019

It to me looks like this was adding rendering of synonyms for existing hospital/dentist etc. tags - which is something we as a principle like to avoid here to support tagging consistency and avoid meaningless proliferation of tags.

+1

Leaving aside the synonyms for existing tags, the new ones don't render properly, so +1 on this PR.

I resolved the conflicts and merged it.

@HolgerJeromin
Copy link
Contributor

Was this part overlooked in the revert?

[feature = 'healthcare_alternative'],
[feature = 'healthcare_audiologist'],
[feature = 'healthcare_birthing_center'],
[feature = 'healthcare_blood_bank'],
[feature = 'healthcare_blood_donation'],
[feature = 'healthcare_centre'],
[feature = 'healthcare_clinic'],
[feature = 'healthcare_dentist'],
[feature = 'healthcare_dialysis'],
[feature = 'healthcare_doctor'],
[feature = 'healthcare_laboratory'],
[feature = 'healthcare_midwife'],
[feature = 'healthcare_occupational_therapist'],
[feature = 'healthcare_optometrist'],
[feature = 'healthcare_physiotherapist'],
[feature = 'healthcare_podiatrist'],
[feature = 'healthcare_psychotherapist'],
[feature = 'healthcare_rehabilitation'],
[feature = 'healthcare_speech_therapist'],
[feature = 'healthcare_yes'] {

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Jan 26, 2020 via email

pnorman added a commit to pnorman/openstreetmap-carto that referenced this pull request Jul 10, 2022
This removes lines that were overlooked in gravitystorm#3731 (comment)
@imagico imagico mentioned this pull request Apr 17, 2023
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.

Healthcare tags do not render on closed ways without area=yes
8 participants