Skip to content

Conversation

@isikl
Copy link

@isikl isikl commented Oct 8, 2019

No description provided.

@isikl isikl requested a review from nilsnolde October 8, 2019 06:56
Copy link
Contributor

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more general review:

Let's first focus on the providers. The sources package makes sense. You can generalize this more even. Each provider should have the same basic structure, i.e. there should be a Base class which each provider inherits from, you can call that Provider.py with a ProviderBase class. You can make that an abstract class, see here if you want some inspiration. And read up a little about abstract classes and methods. It's object-oriented, which Python doesn't really do, so this is more of a hack, but it's an important principle. One abstract method would be download which every subclass has to implement. Then you can define private methods for each provider class to make the download happen.

If you first do that, we can see how we implement the merging and the rest.

I think it's good that you treat SRTM and GMTED as one table in settings.yml and just pull in GMTED when the area demands it. Maybe we can call the table terrestrial instead of srtm.

"DB:\t{db_name}\n"
"Table:\t{table_name}\n"
"Table1:\t{table_name_srtm}\n"
"Table2:\t{table_name_composite}\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is no composite table?

Copy link
Contributor

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isikl after struggling again to make this work in a fresh setup (my fault..), I couldn't get this to work. Mostly because the SETTINGS are used wrongly in most places, e.g.

  File "/deploy/app/openelevationservice/server/db_import/models.py", line 14, in <module>
    if SETTINGS['provider_parameters']['tables']['terrestrial']:
KeyError: 'tables'

If you have a look where SETTINGS is used you'll see various places with non-valid keys according to the ops_settings_sample.yml.

Can you please check and amend?

@isikl
Copy link
Author

isikl commented Jan 13, 2020

Should be fixed now.

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

Successfully merging this pull request may close these issues.

2 participants