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

Rescaling should have been removed in Boost 1.82 #1163

Closed
vissarion opened this issue Jun 19, 2023 · 5 comments · Fixed by #1172
Closed

Rescaling should have been removed in Boost 1.82 #1163

vissarion opened this issue Jun 19, 2023 · 5 comments · Fixed by #1172
Labels

Comments

@vissarion
Copy link
Member

In config.hpp there is still a message about deprecation of rescaling in 1.82.

To my understanding the library is not ready to totally remove rescaling since there are a lot of tests depending on it (they fail without rescaling) and also a lot of issues. Maybe the message is about removing the macro BOOST_GEOMETRY_USE_RESCALING and replace it by BOOST_GEOMETRY_ROBUSTNESS_ALTERNATIVE. If this is the case we can simply remove the message at this step and then in the future work on removing rescaling functionality in general. @barendgehrels am I right?

@barendgehrels
Copy link
Collaborator

barendgehrels commented Jun 19, 2023

Hi @vissarion , good point. It should have been removed.

At this moment, most tests that are marked as failing fail with rescaling. For example, in union, 7 tests fail with rescaling, against 1 which is invalid without. I didn't check all counts, but that was also my general idea.

So the code without rescaling performs currently a lot better, which is why we changed the default. It is also reported, such as recently by Johan Dore in #1145

Only incidentally it is still better for some cases, but now is the time to fix those (at least tried to), instead of recommending the macro.

So we have the options:

  • remove rescaling now. That would be my choice. People have been warned, and they can stay at 1.82 if they still want rescaling
  • change the message to, for example, 1.84

Replacing the macro would not be my choice, because current users (who use it) are used to this name. For me it makes no sense to just rename it.

Being able to remove rescaling has the advantage that the code will become cleaner, it's not just the rescaling code itself, there is also some related code such as in get_clusters or sectionalize.

But of course, there is an effort involved in removing it.

@vissarion
Copy link
Member Author

Thanks @barendgehrels for the detailed reply. I agree that rescaling should be removed. I will try to do it for 1.83 (although the deadline is in one week unless we consider that change a bugfix and not a major change, then there is more time). In any case even if rescaling is not removed in 1.83 the message should be changed.

Replacing the macro would not be my choice, because current users (who use it) are used to this name. For me it makes no sense to just rename it.

I also agree here, but I am a bit confused, what is the point of having BOOST_GEOMETRY_ROBUSTNESS_ALTERNATIVE then? Should that be removed together with BOOST_GEOMETRY_USE_RESCALING?

@vissarion
Copy link
Member Author

Here is a list of issues that are affected by the removal of rescaling and (most probably) need to be fixed.
#1164
#1138
#1103
#1082
#1053
#1044
#1034
#1035

and some old ones that have to be revisited:

#900
#893
#669
#663
#566

@barendgehrels
Copy link
Collaborator

barendgehrels commented Jun 21, 2023

Thanks for this list! There are quite some new ones. I plan to visit them soon.
The ones with integer (for example #1035) actually don't really belong to the list, because integer didn't use rescaling. But it might still be related, somehow.

About BOOST_GEOMETRY_ROBUSTNESS_ALTERNATIVE: yes, I forgot that detail. I introduced it to make the switch easier. It should only be relevant for .bjam files. It selects what is not the default (so the alternative). Earlier it was without rescaling, but now it is with rescaling. It is not meant for the user, though currently it does the same thing indeed. Yes, it can be removed together.

Finally, I would take more time to remove rescaling. It's a lot of work and a week is short. It's up to you of course, but I would not do it last moment. I mentioned remove rescaling now - but I didn't know there was only a week time...

@vissarion
Copy link
Member Author

Yes, indeed one week is short. Let's leave it for 1.84.

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

Successfully merging a pull request may close this issue.

2 participants