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

Rethink qp.PDF.limits #95

Open
Tracked by #94
johannct opened this issue May 25, 2018 · 1 comment
Open
Tracked by #94

Rethink qp.PDF.limits #95

johannct opened this issue May 25, 2018 · 1 comment

Comments

@johannct
Copy link

qp/qp/pdf.py

Line 89 in 7a43a8f

self.limits = (min(self.limits[0], np.min(self.gridded[0])), max(self.limits[-1], np.max(self.gridded[0])))

As limits attribute is provided on top of the grid tuple, I would expect that it means that limits takes over precedence over the grid boundaries. so the lower bound should be the max(grid.min, limits[0]) and the upper bound should be min(grid.max, limits[1])

@aimalz aimalz mentioned this issue May 31, 2018
12 tasks
@aimalz
Copy link
Owner

aimalz commented May 31, 2018

The code is written as I originally intended (it's not a "bug" in the sense of an accident), but I think you're right that it probably shouldn't be that way (not being a bug doesn't make it not a mistake). I did think the notion of PDF-wide "limits" was problematic so circumvented it in the upcoming refactored version. I'll leave this issue open as a reminder, though I'm going to rename it for the sake of context. Thanks for your patience with this.

@aimalz aimalz changed the title gridded limits incorrect? Rethink "limits" from the qp.PDF object May 31, 2018
@aimalz aimalz changed the title Rethink "limits" from the qp.PDF object Rethink qp.PDF.limits May 31, 2018
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

No branches or pull requests

2 participants