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

Field: enable standalone use #166

Merged
merged 12 commits into from
Jun 3, 2021
Merged

Field: enable standalone use #166

merged 12 commits into from
Jun 3, 2021

Conversation

MuellerSeb
Copy link
Member

Minor update to the Field class to enable standalone usage:

  • enable standalone use with given dim
  • add points method to generate field on a point list (transposed unstructured pos tuple as used in many other packages (e.g. PyVista))
  • add latlon property
  • better doc ref to __call__

This was a low hanging fruit. Now Field can be used to display and export arbitrary n-d point data.

…dd 'latlon' property; better doc ref to '__call__'
@MuellerSeb MuellerSeb added the enhancement New feature or request label Apr 18, 2021
@MuellerSeb MuellerSeb added this to the 1.3.1 milestone Apr 18, 2021
@MuellerSeb MuellerSeb requested a review from LSchueler April 18, 2021 19:30
@MuellerSeb MuellerSeb self-assigned this Apr 18, 2021
Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think first of all the __call__ method and maybe also the Field class itself needs more documentation. It is not clear at all what is being "generated" and what the "field values" are.

Now we have Field.structured, Field.unstructured, Field.mesh, and Field.points. Wouldn't it be really nice to enhance the mesh_type keyword to be able to actually use all these possibilities with only one single method call?

gstools/field/base.py Show resolved Hide resolved
@MuellerSeb
Copy link
Member Author

MuellerSeb commented Apr 19, 2021

I think first of all the __call__ method and maybe also the Field class itself needs more documentation. It is not clear at all what is being "generated" and what the "field values" are.

Maybe just a "Set field with position tuple and values" in the doc header?

Now we have Field.structured, Field.unstructured, Field.mesh, and Field.points. Wouldn't it be really nice to enhance the mesh_type keyword to be able to actually use all these possibilities with only one single method call?

This would be an issue for GSTools v2. For now we have to keep these methods. Especially the Field.mesh has a lot of settings where we would need to refactor all derived classes to pass **kwargs needed for mesh handling. By defining these partial calls to __call__ as present, we are really flexible since we only rely on the pos and mesh_type arguments in the __call__ signature (present and needed(!) in all child classes) for all these "shortcut" methods. The "mesh_type" itself is only a leftover from the first implementations of GSTools IMO and I think the "(un)structured" options are enough here (See: #61).

Also the planned xarray interface (see #140) should also be just a new method Field.xarray that uses __call__ internally IMO. Actually I really like this approach to add new interfaces that may need special keywords to control their generation settings (see mesh) but internally then uses the simple __call__ method to generate the desired field. This also makes maintenance more easy, since we never need to adopt __call__ or the mesh_type property and all child classes smoothly inherit these functionalities.

Here we are again discussing the scope of this class 😄

@LSchueler
Copy link
Member

LSchueler commented Apr 21, 2021

Maybe just a "Set field with position tuple and values" in the doc header?

Yeah, together with an example of what that is good for is enough I guess. Something like "This gives you access to e.g. the plotting routines or the normalization functionality."

@LSchueler
Copy link
Member

Here we go again ;-)
The extra args for the mesh method is a good point. But Python is known, loved, and hated for its duck-typing. And now you suggest to create a separate field-generation method for all kinds of different input types? Function overloading would be really handy in this case, but I guess Python lacks that feature for a reason.
structured, unstructured, and points (or whatever the best keyword would be) all have the same arguments.
For me the ideal solution would still be to use the different input types to create a Field class and then do the field generation with only one method call. But I already tried to implement that and didn't finish, because I don't have enough time for all the refactoring. :'(

Anyways, I think the points or point tuples or point list should be added to the mesh_type argument.

@MuellerSeb
Copy link
Member Author

Here we go again ;-)
The extra args for the mesh method is a good point. But Python is known, loved, and hated for its duck-typing. And now you suggest to create a separate field-generation method for all kinds of different input types? Function overloading would be really handy in this case, but I guess Python lacks that feature for a reason.
structured, unstructured, and points (or whatever the best keyword would be) all have the same arguments.
For me the ideal solution would still be to use the different input types to create a Field class and then do the field generation with only one method call. But I already tried to implement that and didn't finish, because I don't have enough time for all the refactoring. :'(

Anyways, I think the points or point tuples or point list should be added to the mesh_type argument.

Then I would vote for kicking points out again, since all it does is transposing the pos tuple. When adding a new mesh_type possibility, we would also need to refactor all downstream routines (exporting, plotting, converting). We added structured and unstructured only for convenience back in the days.

@LSchueler
Copy link
Member

We actually added the structured and unstructured methods for convenience, not the mesh_type keywords. However, we shouldn't use history as arguments for the future development ;-)

I have to agree that it would take a lot of careful considerations, if we introduce a new mesh_type keyword. Although I still do prefer it that way. But if we now introduce a method points and later maybe remove it or add mesh_type keywords, that could get pretty confusing. Maybe we should indeed drop the points method for the moment.

@MuellerSeb
Copy link
Member Author

Ok, then I am fine with dropping points again. ;-) I think passing a points.T for pos is also fine.

@MuellerSeb
Copy link
Member Author

@LSchueler removed the points method and added an example. LGTM!

Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's good to have the example.
And I think we're nearly there ;-)

CHANGELOG.md Show resolved Hide resolved
examples/00_misc/05_standalone_field.py Outdated Show resolved Hide resolved
examples/00_misc/05_standalone_field.py Show resolved Hide resolved
gstools/field/base.py Outdated Show resolved Hide resolved
Copy link
Member

@LSchueler LSchueler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We finally made it! 👍

@MuellerSeb
Copy link
Member Author

We finally made it! +1

Yeah! Will merge when CI is successful. 🎉

@MuellerSeb MuellerSeb merged commit 94b006a into develop Jun 3, 2021
@MuellerSeb MuellerSeb deleted the field_update branch June 3, 2021 13:43
@MuellerSeb MuellerSeb mentioned this pull request Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants