-
Notifications
You must be signed in to change notification settings - Fork 168
Support for creating a ParticleSet with empty FieldSet #880
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
Conversation
CKehl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adaptations appear to be complete and the unit tests all pass, so that's good. I think with the changes in 'test_kernel_language.py', it becomes apparent why this is a good idea - no matter what we do to the FieldSet at some point, those tests are not affected by it. And on the other hand, if there is a question if a fail is die to FieldSet or ParticleSet, this class now gives ample info to decide on that question.
| and propagates attribute access.""" | ||
|
|
||
| def __init__(self, fieldset, ptype): | ||
| def __init__(self, fieldset=None, ptype=JITParticle): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, so you decided to keep this parameter order, and then also rather just define 'JITParticle' as ptype standard. Yeah, fine - relieves you from a lot of tiny adaptions in the other files, I agree.
| logger.warning_once("No FieldSet provided in ParticleSet generation. " | ||
| "This breaks most Parcels functionality") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to set a warning. The formulation is very user-centric, but that's probably how it should be.
| for v in self.ptype.variables: | ||
| if v.name in ['xi', 'yi', 'zi', 'ti']: | ||
| self.particle_data[v.name] = np.empty((len(lon), fieldset.gridset.size), dtype=v.dtype) | ||
| ngrid = fieldset.gridset.size if fieldset is not None else 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't that be 0 if no fieldset is given ?
| if v.name in ['xi', 'yi', 'zi', 'ti']: | ||
| self.particle_data[v.name] = np.empty((len(lon), fieldset.gridset.size), dtype=v.dtype) | ||
| ngrid = fieldset.gridset.size if fieldset is not None else 1 | ||
| self.particle_data[v.name] = np.empty((len(lon), ngrid), dtype=v.dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, okay, now I see why 'ngrid' should not be 0 ... the variable naming semantic here is a bit misleading then, but I understand the purpose.
…nelgenerator Support for creating a ParticleSet with empty FieldSet
This PR support creating a
ParticlseSetwithout aFieldSet, or withfieldset=None.This fixes #867 and addresses #618, to deconvolute Parcels class dependencies