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

Add better support for custom generators #250

Merged
merged 18 commits into from
Aug 16, 2022
Merged

Add better support for custom generators #250

merged 18 commits into from
Aug 16, 2022

Conversation

LSchueler
Copy link
Member

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 LSchueler requested a review from MuellerSeb August 6, 2022 13:15
@LSchueler LSchueler self-assigned this Aug 6, 2022
@LSchueler LSchueler added the enhancement New feature or request label Aug 6, 2022
@LSchueler
Copy link
Member Author

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

@MuellerSeb
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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 MuellerSeb added this to the v1.4 milestone Aug 10, 2022
Copy link
Member

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

@LSchueler I think this looks good now.

@LSchueler
Copy link
Member Author

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
Copy link
Member

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.

src/gstools/field/generator.py Outdated Show resolved Hide resolved
src/gstools/field/srf.py Outdated Show resolved Hide resolved
src/gstools/field/cond_srf.py Outdated Show resolved Hide resolved
Copy link
Member

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

LGTM

@LSchueler LSchueler merged commit 4362175 into main Aug 16, 2022
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