GSTools icon indicating copy to clipboard operation
GSTools copied to clipboard

Add better support for custom generators

Open LSchueler opened this issue 3 years ago • 5 comments

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 avatar Aug 06 '22 13:08 LSchueler

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

LSchueler avatar Aug 06 '22 14:08 LSchueler

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 avatar Aug 09 '22 12:08 MuellerSeb

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 avatar Aug 09 '22 12:08 MuellerSeb

Just added the mentioned enhancements:

  • abstract generator class AGenerator (self-made generators should inherit from that)
  • checks for subclass of AGenerator when setting generators
  • valid_value_types class variable for all Field classes

MuellerSeb avatar Aug 10 '22 15:08 MuellerSeb

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.

LSchueler avatar Aug 15 '22 17:08 LSchueler

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.

MuellerSeb avatar Aug 16 '22 12:08 MuellerSeb