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

Suggestion: move preliminary calculations to module initialization #41

Open
efvik opened this issue Oct 23, 2024 · 2 comments
Open

Suggestion: move preliminary calculations to module initialization #41

efvik opened this issue Oct 23, 2024 · 2 comments

Comments

@efvik
Copy link
Collaborator

efvik commented Oct 23, 2024

Initializing the TimeSeries module currently just sets the variables. I think it would be better to restructure and perform most of the preparations at initialization, rather than in import_data. For example:

  • get date range,
  • get nearest location to coordinates,
  • generate urls
  • test that we can access first and last file

This way we can also print (optional) feedback before downloading is initialized, for example:

  • how many files will be accessed,
  • the time range and number/frequency of timestamps,
  • which location was found and distance+direction from the requested coordinates,
  • approximate size of the dataset, or approximate time to download

This would make it easier to use the api for someone unfamiliar with a product, and make it possible to debug or adjust the request before starting a download.

@lassebje
Copy link
Collaborator

lassebje commented Oct 24, 2024

Personally I like the simplicity of the TimeSeries class. It is one point, one continuous time range. I agree with a lot of your feature request, but instead of having everything into the constructor, it would in my opinion be beneficial to add several methods. See ongoing PR https://github.com/MET-OM/metocean-api/blob/55c4f48db5788321e5f37084b1a5524556aa37fe/metocean_api/ts/ts_mod.py where I suggest adding a download method. It is just as easy to add a get_urls and or an initialise method that prefetches and does all the setup neded before doing the actual downloads. Doing a "lot of expencive stuff" in the constructor (such as going online) is probably not a good idea because it makes the class less versatile.

@efvik
Copy link
Collaborator Author

efvik commented Oct 24, 2024

I think just accessing a file without downloading anything is very inexpensive, less than a second. And that it would make sense to check that the files exist and the server is available. But I agree that separating the process into disctinct methods is a good idea. A separate "verification" method could be added, to be run manually by users if they want feedback on the data they have selected before downloading. It would be optional and not interfere with the rest of the system. This is a low priority feature though, and I guess it doesn't make sense to add while the project structure is changing.

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