Removing YTSphereTooSmall #5211
Conversation
cphyc
added
discussion
|
Looks like many test cases actually rely on the implied indexing... |
|
@neutrinoceros yes... I've gone ahead and triggered the creation of the index manually. This isn't super nice, but I don't have the knowledge to touch on the volume rendered. Maybe @chrishavlin would want to have a stab at it? |
| @@ -57,12 +57,6 @@ def __init__( | |||
| super().__init__(center, ds, field_parameters, data_source) | |||
| # Unpack the radius, if necessary | |||
| radius = fix_length(radius, self.ds) | |||
| if radius < self.index.get_smallest_dx(): | |||
There was a problem hiding this comment.
isn't this where we should be adding a self.ds.index for backward compat ?
There was a problem hiding this comment.
I don't think so. We do not want the index to be computed unless strictly necessary (this is a costly operation). If we add self.ds.index here, anytime you build a sphere, you'll have an index created along the way, which you don't with ds.region for example.
Note that I agree this is the current behaviour, but removing it is one of the motivation behind this PR :)
There was a problem hiding this comment.
I agree with @cphyc. Taking this out makes these objects significantly lighter.
There was a problem hiding this comment.
Of course, but the fact that we needed to modify tests indicates there's a very high chance it also breaks user code, doesn't it ?
I agree we should aim at delaying this expensive operation, but I don't think this PR is the correct time to do it.
There was a problem hiding this comment.
To me, the issue is that we have some public APIs that require the index to be built — ds.field_info being one of them — yet they don't all trigger the creation of the index.
There was a problem hiding this comment.
I agree. This is the root problem. I still don't think we should knowingly break existing users before we form a better plan
There was a problem hiding this comment.
I think these tests are not reflecting real usage. One would never really access the field info container in this way. That said, the brittleness of field_info needing other actions before it can be access has always bugged me. PR #5250 is a potential fix for this that would allow us to remove the ds.index calls in these tests.
There was a problem hiding this comment.
One would never really access the field info container in this way.
end users certainly wouldn't, but can we say this with confidence about downstream apps and libraries ?
There was a problem hiding this comment.
No guarantees, but it's hard to imagine. That this previously worked was a side-effect of suboptimal functionality. This is a potentially huge performance improvement. I'm in favor of merging this whether or not it breaks things downstream.
There was a problem hiding this comment.
I'm fine with seeing this go. Data objects should be as light as possible upon creation. If we wanted to retain functionality of this nature, I think it would be better to do it when fields are queried. I'm not convinced we need to do that and I certainly don't think it should happen here.
PR Summary
The check on grid size cause the building of the entirety of the index.
This causes a massive slowdown for e.g. RAMSES dataset where the smallest cell size requires reading the entirety of the AMR structure.
The fix? Remove this check and live on the edge.
Note: this makes #5060 more or less useless.