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

BUG #1739: fitToViewport uses dataset bounds instead of recomputing them #1744

Merged
merged 1 commit into from Jan 9, 2016

Conversation

@danlipsa
Copy link
Contributor

@danlipsa danlipsa commented Dec 23, 2015

fitToViewport used to recompute the dataset bounds using an 2D array of points.
This was slow and imprecise. Now we pass the dataset bounds as parameters.

@danlipsa
Copy link
Contributor Author

@danlipsa danlipsa commented Dec 23, 2015

This is still work in progress. I need to replace all fitToViewport with the bounds version.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 4, 2016

@danlipsa do you want to post update to all of us?

@danlipsa danlipsa force-pushed the projected-data-bounds branch 2 times, most recently from e3b9847 to d1b4997 Jan 5, 2016
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 6, 2016

fitToViewport recomputes the dataset bounds using an 2D array of points.
This is slow and imprecise and it does not take into account the bounds stored
in the file. Now we use a new function fitToViewportBounds that
receives the dataset bounds as parameter. The old function is still used for
the outline and the continents.  It is going to be removed in the future.
@danlipsa danlipsa force-pushed the projected-data-bounds branch from d1b4997 to 1583a77 Jan 6, 2016
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 7, 2016

@doutriaux1 did you review this branch?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 7, 2016

Not yet, was working on bots and lambert yesterday.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 8, 2016

@doutriaux1 let's merge this one as well soon so that we can release it today.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 8, 2016

there's no release w/o projections. Will take a look at this one when proj are in better shape.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 8, 2016

there's no release w/o projections. Will take a look at this one when proj are in better shape.

@doutriaux1 lets talk about this in the afternoon. I don't understand what you are saying here. Based on our discussions, we should release as things are better and not more broken than before.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 8, 2016

all i'm saying is I want to give a fair shot to the projection part, if we keep adding bits in the mean time it makes it harder to fix the proj issue. I will take a look at tihs when we fix or quit the proj issues

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 8, 2016

@aashish24 @danlipsa ok this branch look good, but there's going to be baseline conflicts with lambert branch (whichever you decide on). LEt's merge lambert first and then update again this PR. Ok?

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 8, 2016

once lambert is in master I will trigger a rebuild of this one so we can see which files need updating.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 9, 2016

@aashish24 @danlipsa ok this branch look good, but there's going to be baseline conflicts with lambert branch (whichever you decide on). LEt's merge lambert first and then update again this PR. Ok?

@doutriaux1 we should merge this branch first as this was pushed before the lambert one. It would be nice if we follow the queue approach. Also, the lambert is the small one. I would prefer if you merge this one first.

doutriaux1 added a commit that referenced this issue Jan 9, 2016
BUG #1739: fitToViewport uses dataset bounds instead of recomputing them
@doutriaux1 doutriaux1 merged commit 6f64cc6 into master Jan 9, 2016
3 of 8 checks passed
@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Jan 9, 2016

sure...

@doutriaux1 doutriaux1 deleted the projected-data-bounds branch Jan 9, 2016
@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Jan 9, 2016

@doutriaux1 did I say that you are awesome!!

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

Successfully merging this pull request may close these issues.

None yet

3 participants