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

Refactor find_besthist #27

Open
kamwal opened this issue May 12, 2020 · 6 comments
Open

Refactor find_besthist #27

kamwal opened this issue May 12, 2020 · 6 comments
Assignees
Labels
ignore-for-release Ignore this for next release refactoring improve the design, structure, and/or implementation

Comments

@kamwal
Copy link
Contributor

kamwal commented May 12, 2020

Currently, the find_besthist function requires 18 arguments to run. This is far too many, looks a bit messy, and can make using the function rather difficult.

I suggest taking some of the arguments out and placing them in a class, especially where the same class can be used for differing data.

Create a class for containing float data. This class can contain the latitudes, longitudes, z vales, and dates of different data points. We can either create two objects (one being the float, and the second being all the historical points), or we can create an array of objects, where each object is a single data point. This class could also contain some functions that are exclusively used on the data (i.e. calculating potential vorticity).

We could also create a class for calculating the ellipse that inherits from the other class to remove even more arguments.

I also think we should just pass in the struct containing all the configuration variables into the function, rather than dropping them in one by one.

Doing all the above will reduce the argument count from 18 to 3. I would be much more manageable.

@kamwal kamwal changed the title Refactor find_besthist so that it doesn't require 18 arguments Refactor find_besthist May 12, 2020
@gmaze
Copy link
Member

gmaze commented May 12, 2020

Create a class for containing float data. This class can contain the latitudes, longitudes, z vales, and dates of different data points. We can either create two objects (one being the float, and the second being all the historical points), or we can create an array of objects, where each object is a single data point. This class could also contain some functions that are exclusively used on the data (i.e. calculating potential vorticity).

This could be made using the github.com/euroargodev/argopy data model that comes with this kind of functionalities already

@edsmall-bodc
Copy link
Collaborator

@gmaze That's a great idea. Is it the ArgoDataFetcher class you are thinking of in particular? Correct me if I'm wrong, but it looks like we can collect float data from a square region by specifying latitude and longitude, or using WMO boxes?

If that's correct, we can actually get rid of the WMO boxes too, and fetch all our data using this, as long as we can remove the profile currently being analysed from the fetched data set. Shouldn't be hard, as it looks like this fetches the profile names too.

The only issue I am foreseeing is that this only fetches argo data, not CTD/Bottle data too?

@gmaze
Copy link
Member

gmaze commented May 12, 2020

With the ArgoDataFetcher, you can retrieve data from the Argo reference dataset (last 2019V03 version available in the upcoming days)
The goal of this feature was precisely to make it more simple to request reference data from QC software like OWC ! And yes, it's trivial to make sure the profile currently analysed is not part of it.

The WMO boxes are a non sense in a modern software framework.

And yes CTD/bottle data are not available now, since this would require some kind of authentication system that we are currently working on.

Have a look to: https://github.com/euroargodev/erddap_usecases/blob/master/examples/Example-Standard-02.ipynb, and even run it online with Binder.

@edsmall-bodc
Copy link
Collaborator

@gmaze I see there are some plotting capabilities as well - I think we should chat at some point about the current OWC output to see if any of your plotting routines can be used here?

@gmaze
Copy link
Member

gmaze commented May 12, 2020

not yet very developed since it's not the core focus of the library (which is to fetch and manipulate argo data), but working on notebooks and visualisation examples
yes, surely a lot of overlap here

@edsmall-bodc
Copy link
Collaborator

edsmall-bodc commented May 12, 2020

I guess, for now, I can make the plotting routines that exist in the matlab version in python, with a view to potentially make an argo plotting package for all argo focused plotting routines to exist

@gmaze gmaze transferred this issue from euroargodev/User-Acceptance-Test-Python-version-of-the-OWC-tool Jul 7, 2020
@gmaze gmaze added the refactoring improve the design, structure, and/or implementation label Jul 7, 2020
@kamwal kamwal added the ignore-for-release Ignore this for next release label Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release Ignore this for next release refactoring improve the design, structure, and/or implementation
Projects
None yet
Development

No branches or pull requests

3 participants