GeoStat-Framework / GSTools

GSTools - A geostatistical toolbox: random fields, variogram estimation, covariance models, kriging and much more
https://geostat-framework.org
GNU Lesser General Public License v3.0
567 stars 74 forks source link

Add better support for custom generators #250

Closed LSchueler closed 2 years ago

LSchueler commented 2 years ago

At the moment it is very difficult to use own or modified random generators in the SRF and CondSRF classes, because we only support using the lookup tables GENERATORS. This PR adds the possibility again to hand over own classes. I added some error checking, ensuring that the class is a subclass of RandMeth. This is not very pythonic, but using duck typing here is pretty tricky. For example, handing over SRF as the generator gives this error only when trying to actually create an srf:

in SRF.__call__(self, pos, seed, point_volumes, mesh_type, post_process, store)
    144 name, save = self.get_store_config(store)
    145 # update the model/seed in the generator if any changes were made
--> 146 self.generator.update(self.model, seed)
    147 # get isometrized positions and the resulting field-shape
    148 iso_pos, shape = self.pre_pos(pos, mesh_type)

AttributeError: 'SRF' object has no attribute 'update'

That's why I opted for the strick instance checking. What are your thoughts in this approach?

LSchueler commented 2 years ago

The failing checks will be successful as soon as PR #248 is merged.

MuellerSeb commented 2 years ago

I like this. We should have an abstract base class for generators, since they don't need to be based on the randomization method. In this ABC we could then define all needed methods and properties that should be provided for the SRF classes.

This should be tested.

MuellerSeb commented 2 years ago

And one problem I noticed: The value_type needs to be restricted better then. Since the vector version of RandMeth is not valid for CondSRF, the valid value types should be a class variable (and not a parameter VALUE_TYPES value in the field/base.py module). So I would suggest to add valid_value_types = ["scalar", "vector"] at the beginning of Field and overwrite it in Krige and CondSRF (there it is only need to be explicit, since the underlying kriging class would check the value after setting it in set_generator).

MuellerSeb commented 2 years ago

Just added the mentioned enhancements:

LSchueler commented 2 years ago

Thanks for the input! The only thing I don't agree with, is your naming convention of the abstract Generator class. I think I have never seen them with a capital A and I don't think that we need do have extra naming conventions for ABCs. Do you agree? - If yes, I would merge this PR.

MuellerSeb commented 2 years ago

Thanks for the input! The only thing I don't agree with, is your naming convention of the abstract Generator class. I think I have never seen them with a capital A and I don't think that we need do have extra naming conventions for ABCs. Do you agree? - If yes, I would merge this PR.

I just copied this naming convention from another project, where I am collaborating and I thought this was a thing. But I agree that Generator reads better and will be more clear for our use cases. I will apply the mentioned fixes from above and merge then.