refactor: minor change to Mars dataset downloading logic. - #503

Thanks for the PR. It looks good, but I'll need about 2 weeks before I can get to this. After we agree on this, we'll have to make changes to a couple similar datasets that do the same thing.

Thanks for the PR. It looks good, but I'll need about 2 weeks before I can get to this. After we agree on this, we'll have to make changes to a couple similar datasets that do the same thing.

@MarkWieczorek Thanks, I can get started on applying this to other datasets if you'd like. I see a total of nine datasets which offer multiple resolutions.

Just let me know which of these two implementations you'd prefer:

1. Copy-paste the full logic from my PR into each existing loading function, or
2. Move all of this logic into a new function (perhaps in pyshtools/utils/), which would take a generic pooch.Pooch object with an existing registry, and return the appropriate filename.

The latter is more abstracted/complicated, but since it would apply to nine datasets, it would hopefully make future adjustments much easier (cleanness and aesthetics are a bonus). Like I said I'm happy to implement either one, just want to avoid stepping on any toes.

Thanks again for your time.

Thanks for your effort on putting this together, and sorry about how long it took to get back to you.

I am thinking that it might be best to create some kind of small helper function to find out if a suitable local file already exists. But instead of putting it in the utils module, what if we just created a hidden function in the pyshtools/datasets/__init__.py file? This won't be used for anything else outside of the datasets module, so I don't think we really need to put it in a different module. Also, at some point in the future I am thinking of breaking this project into several smaller projects (python classes, fortran backend, and datasets+constants). Keeping this function in the datasets module would help for when we that happens.

I just have one comment about this comment in the code:

Note that from python 3.6 onwards, dictionaries maintain order of
declaration/insertion, so this will go from largest model to smallest.

I think that you are assuming that the registry will always be ordered like in this example. Maybe that is ok, but I am just worried that in the future someone might use an inverted order. Perhaps we shouldn't make any assumptions on the ordering.

What do you think?

@MarkWieczorek Apologies for the delay. I've cleaned up this PR so it's a single commit with all intended changes.

To summarize (since it's been a while):

• The goal of this PR is to modify downloading logic for datasets with multiple resolutions to reuse existing high-res model files instead of unnecessarily downloading additional low-res model files.
  • For example, let's say you've called MOLA_shape(2000) which downloaded the degree 2,879 model. If you then call MOLA_shape(1000), pyshtools will download the 1,439 model. We would save space/time to reuse the previously downloaded 2,879 model, which is what this PR aims to implement.
• Initially, this PR only applied the change to Mars.MOLA_shape(...), with logic written directly into the function.
• Now, based on your feedback, this PR applies the change to all datasets with multiple resolutions via a helper function _choose_sh_model(...).
  • In your previous comment (April 27), you suggested creating a helper function in pyshtools/datasets/__init__.py. However, this leads to circular import errors $^{\dagger}$ unless you define the helper function at the very top of __init__.py, which would push existing imports down ~150 lines. Aesthetically, this doesn't feel right to me, so I placed the helper function in a new file pyshtools/datasets/_utils.py — let me know if that's okay with you.
  • In your previous comment (April 27), you suggested the helper function should NOT assume the filenames in the registry are already sorted from largest to smallest max degree. Therefore, _choose_sh_model(...) automatically determines the degree of each model by assuming all filenames are identical except for a single integer which is taken as the degree. For example, given two filenames Planet_DEM128_314.sh.gz and Planet_DEM128_2718.sh.gz, we identify the degrees as 314 and 2,718 respectively.

Everything builds just fine in a docker container, make check passes, and the full output of make python-tests is pasted here if you'd like to review: https://gist.github.com/Humboldt-Penguin/abdf2e2d10f8a8a615669e67666cdcda

If everything looks good on your side, this should be ready to merge. I'm happy to make any further tweaks (definitely more timely than before), just let me know.

I'm starting to look at this (finally), and will accept this before putting out a new release next week. However, I'm confused why this PR didn't trigger the CI tests. Could you rebase this on the current develop branch and see if this triggers the tests? If not, don't worry about...

Ok. I'll merge this. I think an easier way would be to modify Pooch so that it allows one to add arbitrary metadata for each file in the registry. For our case, the only metadata that really matters is the lmax that is encoded in the filename. I'll bring this up with the Pooch maintainer. It should be as simple as adding an optional metadata dictionary.