search

ldeo-glaciology / xapres

package for processing ApRES data using xarray
MIT License
3 stars 2 forks source link

should ApRESDEFs.dB in utils.py #16

Closed jkingslake closed 1 year ago

jkingslake commented 1 year ago

@glugeorge, should we put the method dB in your new utils.py file? Currently it is in ApRESDEFs: https://github.com/ldeo-glaciology/xapres_package/blob/873e222ded26b9695bb4c2ecc8692428b3e99ab7/xapres_package/ApRESDefs.py#L452

glugeorge commented 1 year ago

I think that depends how we want to package the final product. I put phase2range in utils since it's kind of a helper function that can be called by multiple classes if needed, but a front-end user would be unlikely to use it if they imported xapres as a package.

On the other hand if they imported a future package that packages ApRESDefs as xa, they can freely use xa.dB to make the relevant plots. I would need to think a bit further and explore how this structuring works.

jkingslake commented 1 year ago

So how would one call phase2range currently? And why wouldnt an end user want to use it? Because it is more for internal use by higher level functions?

glugeorge commented 1 year ago

So right now, it would be called by importing the utils folder and calling it directly. However, if the code is all packaged up into a package like numpy or xarray, I'm not sure if that's possible anymore.

And yes, precisely, it's only use right now is in generating the range calculations, which calls it internally. If you think it could be useful as a function available to the end user, we can move it for sure.

jkingslake commented 1 year ago

Yeah I don't understand how the loading of different modules works very well.

I did make the last version of the code into a package, so you can pip install it (see readme). So maybe we can experiment a bit. But now we have updated it maybe we need to make another release. I'm not sure.