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

Abstraction #14

Closed
prisae opened this issue Apr 29, 2018 · 5 comments · Fixed by #85
Closed

Abstraction #14

prisae opened this issue Apr 29, 2018 · 5 comments · Fixed by #85
Labels
maintenance Maintaining status quo
Milestone

Comments

@prisae
Copy link
Member

prisae commented Apr 29, 2018

Create main classes

  • survey,
  • model, and
  • settings,

where settings might be part of survey. Have a look at SimPEG and a chat with the SimPEG folks to discuss this.

@prisae prisae added this to the v2.0.0 milestone Apr 29, 2018
@prisae prisae added the maintenance Maintaining status quo label Apr 29, 2018
@prisae
Copy link
Member Author

prisae commented Apr 29, 2018

Current input parameters would go into these classes:

survey model settings
src depth xdirect
rec res ht
msrc aniso htarg
mrec epermH ft
strength epermV ftarg
srcpts mpermH loop
recpts mpermV parallel
freqtime
signal
ab

Most of the utils-functions would also go into these classes.

@prisae
Copy link
Member Author

prisae commented May 1, 2018

At the moment, empymod exists without classes. With two exceptions: (1) Sub-classing ndarray to add amp and pha attributes, and (2) the DigitalFilter-class which is a dummy to store values, could be done with a dict too.

One of the reasons why I chose to do it that way is that I think code like this is so much easier to understand by someone coming new to a project. Easier to read, easier to understand, easier to follow. Almost all arguments are passed explicitly, you know exactly what goes into a function. With some lists and dicts this isn't strictly true either, but for instance in empymod.kernel you can look at the function-signature and know exactly what is used, without inspecting a class-object.

It probably goes a bit along the lines of the controversy talk by Jack Diederich: https://www.youtube.com/watch?v=o9pEzgHorH0 (PYCON US 2012).

Now, I am thinking that empymod v2.0.0 will be backwards incompatible (although without breaking the main routines model.dipole, model.bipole, and model.analytical), and take the opportunity to clear-out and improve things that I would do differently today. One of them would be to use a few classes, for instance a Survey and a Model class, and probably a Settings class, although that could go into the Survey.

Now, I am not sure yet if I follow this road, if I will change everything under the hood for more abstractions, and what an eventual timeline would look like. I am merely at a brainstorming point.

@prisae
Copy link
Member Author

prisae commented Oct 13, 2019

More and more I think this is not the way to go. empymod is a small package, and its strength is simplicity. We can do something like

import empymod 
EMfield = empymod.model.dipole( 
    src=[0, 0, 100], 
    rec=[1000, 0, 200], 
    depth=[0, 300, 1000, 1050], 
    res=[2e14, 0.3, 1, 50, 1], 
    freqtime=1 
) 

So I think the functional approach is appropriate, good enough, or just fine for its purpose.

However, one thing to think about is using the new dataclass feature internally. And generally, reduce, e.g.,

def dipole(src, rec, depth, res, freqtime, signal=None, ab=11, aniso=None,
           epermH=None, epermV=None, mpermH=None, mpermV=None, xdirect=False,
           ht='fht', htarg=None, ft='sin', ftarg=None, opt=None, loop=None,
           verb=2)`

to

def dipole(src, rec, depth, res, freqtime, signal=None, ab=11, aniso=None, **kwargs):

or similar.

@prisae
Copy link
Member Author

prisae commented Feb 22, 2020

Also, bipole, dipole, loop, analytical, dipole_k, and gpr should be combined into a more general routine that can handle all cases.

@prisae prisae mentioned this issue Mar 11, 2020
12 tasks
prisae added a commit that referenced this issue Mar 12, 2020
@prisae prisae linked a pull request Mar 13, 2020 that will close this issue
@prisae
Copy link
Member Author

prisae commented Mar 13, 2020

It stays mostly as it is. empymod is a small routine, and fine as it stands. Abstraction would require a huge re-write, and make it much less readable. kwargs was introduced in v2. An overarching general modelling routine might be an idea at some point, but no must. That's it for #14.

prisae added a commit that referenced this issue Mar 13, 2020
All settings (`xdirect`, `ht`, `htarg`, `ft`, `ftarg`, `loop`, `verb`) are now extracted from `kwargs`. This makes it possible that all model-functions take the same keyword-arguments; warnings are raised if a particular parameter is not used in this function, but it doesn't fail (it fails, however, for unknown parameters). Pure positional calls including those parameters will therefore not work any longer. Closes #14
@prisae prisae closed this as completed in eeba70b Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Maintaining status quo
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant