greglucas/pysecs

Release v0.1

Opened this issue · 3 comments

This is a list of things to do before releasing a more stable version.

  • Compare against original code #4.
  • Add more tests to cover additional cases.
  • Update readme/installation instructions.
  • Add curl-free magnetic field routine (T_cf currently raises NotImplementedError). I need to work out the math here still.
  • Add some example scripts/gallery plots in the documentation.
  • Add references to projects that have used this work.
  • Consider getting a DOI for the code to be citable.

@bmurphy-usgs as a heads up. I went through and changed a lot of documentation last night, which included the name of the repo itself. I decided to lowercase everything to make it more Pythonic.

So, you should be able to install with pip install pysecs now. And then import pysecs and everything should work the same as before.

@greglucas , I would like to turn https://github.com/usgs/geomag-imp into as thin a wrapper for pysecs as possible, and just make pysecs a dependency for geomag-imp. Your implementation is just so much cleaner and seemingly more efficient.

But, I don't want to break backward compatibility for those (mostly SWPC) who've already adopted geomag-imp. Specifically, I want to retain the kernel/regressor design so that it resembles ScikitLearn's Gaussian Process tools.

My inclination is to create an object inside my own SECS class named something like _pysecs_SECS, which would be a pysecs.SECS class. Then I would re-implement all my methods as wrappers to this where functionality was shared.

What do you think about this? First, does this sound like a reasonable approach to you? And second, do you feel like pysecs is stable enough to make it a dependency?

@erigler-usgs, I am completely fine with however you want to approach using the code. I put it under an MIT license, so you could just copy over the portions you want and retain the copyright notice to satisfy your work too. The one big difference between our codes is that I put all of the times into an extra dimension of the arrays for numpy to be able to vectorize the routines over time in addition to space. So, if someone is always calling your routines with just a single snapshot in time, there really won't be much efficiency gain anyways. Bottom line, I think you'll need to change the input call structure to take advantage of any speedups anyways, which would be a breaking change on your end already I think.

Right now, I think pysecs is pretty stable, we just need to make sure that the dimension ordering and things don't change. I tried to be consistent with your design with the (lat, lon, r) ordering, which works for me, but is one thing we will want to make sure we don't arbitrarily change in the future (i.e. do we want to go with spherical coordinates and order it with r first?).

If you do want to take advantage of this code, I think the way you describe about making an internal _SECS pointer to a pysecs.SECS instantiated class would be an OK way to do it and then in your routines you would just inernally route the calls through like _SECS.fit() with the proper shaped arrays (adding a time dimension). Probably requiring you to also create properties for the amps that looks something like

@property
def amps(self):
    return self._SECS.sec_amps