FEAT: Add mesh properties, fail on mismatched meshes - #2848
FEAT: Add mesh properties, fail on mismatched meshes#2848VeckoTheGecko wants to merge 7 commits into
mesh properties, fail on mismatched meshes#2848Conversation
|
if you have capacity @wyatt-fluidnumerics , it would be great to have your review |
VeckoTheGecko
changed the title
mesh properties, and fail on mismatched meshesmesh properties, fail on mismatched meshes
|
|
||
| def __init__(self, models: list[ModelData]): | ||
| if models == []: | ||
| raise ValueError("List of models can't be empty.") |
There was a problem hiding this comment.
With this change we can't have fieldsets with only constant fields (which is very reasonable), since that would result in an ambigious fieldset mesh.
Hence I also removed test_fieldset_time_interval_constant_fields
There was a problem hiding this comment.
I realise now that our docs relied on "empty, constant only" fieldsets.
docs/user_guide/examples/tutorial_interaction.ipynb
I assume that you wrote this @erikvansebille . I'll postpone merging, and leave this open until you're back
There was a problem hiding this comment.
Actually, I think there are use cases where we have empty fieldsets - like indeed in the particle-particle interaction tutorial. If users want to use parcels as an Agent-Based-Model engine, they typically don't have/use Fields.
How problematic is it to support empty fieldsets? Perhaps we should change the interaction-tutorial to not have any fields at all (instead of only constant fields)? Or will that be a major endeavour (for another PR)?
There was a problem hiding this comment.
If users want to use parcels as an Agent-Based-Model engine, they typically don't have/use Fields.
I'm not quite sure I understand this - I feel that this is scope creep for the project. Why would users want to use Parcels as an engine for agent based modelling? (instead of other libraries in Python for this)
wyatt-fluidnumerics
left a comment
wyatt-fluidnumerics
left a comment
There was a problem hiding this comment.
This all looks great to me. I think this would also be a good time to remove the mesh argument from add_constant_field like you mentioned in the original issue.
Yeah sure, I can wrap that into here :) |
Actually, I remember why I didn't - I wanted to do it after #2845 as that would make things easier (I also wanted to expand testing for Separate PR would be better I think |
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good - except that I think we should support empty FieldSets. but that could perhaps be annother PR? See below
|
|
||
| def __init__(self, models: list[ModelData]): | ||
| if models == []: | ||
| raise ValueError("List of models can't be empty.") |
There was a problem hiding this comment.
Actually, I think there are use cases where we have empty fieldsets - like indeed in the particle-particle interaction tutorial. If users want to use parcels as an Agent-Based-Model engine, they typically don't have/use Fields.
How problematic is it to support empty fieldsets? Perhaps we should change the interaction-tutorial to not have any fields at all (instead of only constant fields)? Or will that be a major endeavour (for another PR)?
3 participants
Description
Changes:
ModelDatabase classassert_models_have_same_mesh(which raisesIncompatibleMeshesException)Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
None used