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

Release v0.1 #6

Open
3 of 7 tasks
greglucas opened this issue Mar 11, 2020 · 3 comments
Open
3 of 7 tasks

Release v0.1 #6

greglucas opened this issue Mar 11, 2020 · 3 comments
Milestone

Comments

@greglucas
Copy link
Owner

greglucas commented Mar 11, 2020

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

  • Compare against original code Test against original Matlab 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.
@greglucas greglucas added this to the v0.1 milestone Mar 11, 2020
@greglucas
Copy link
Owner Author

@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.

@erigler-usgs
Copy link
Collaborator

@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?

@greglucas
Copy link
Owner Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants